On 2/13/22 16:26, Alvaro Herrera wrote:
> On 2022-Feb-12, Tomas Vondra wrote:
>
>> Fix skip-empty-xacts with sequences in test_decoding
>>
>> Regression tests need to use skip-empty-xacts = false, because there
>> might be accidental concurrent activity (like autovacuum), particularly
>> on slow machines. The tests added by 80901b3291 failed to do that in a
>> couple places, triggering occasional failures on buildfarm.
>
> I think you meant "... need to use skip-empty-xacts = true", since you
> changed the value from 0 to 1, and the point is precisely to *skip*
> those transactions. (The hidden double negative "skip=false" seems
> quite confusing BTW.)
>
Yeah, that's what I meant :-(
>> Fixing the tests however uncovered a bug in the code, because sequence
>> callbacks did not handle skip-empty-xacts properly. For trasactional
>> increments we need to check/update the xact_wrote_changes flag, and emit
>> the BEGIN if it's the first change in the transaction.
>
> Hmm. Perhaps there should be a separate test script that runs various
> things under skip-empty-xacts=0 and somehow protected against ancillary
> activities (maybe a TAP test using autovacuum=0), just so that this new
> code is covered ...
>
I'm not sure what exactly would be the benefit? I mean, it'd be testing
exactly the same thing as now - actually less than now, because instead
of ignoring empty transactions in the last step (output plugin), we'd
make sure not to have any empty transactions.
For the record, the new code in sequence decoding is tested already, at
least for transactional increments.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
