Thread: pgsql: Fix skip-empty-xacts with sequences in test_decoding
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. 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. Reported-by: Andres Freund Discussion: https://postgr.es/m/20220212220413.b25amklo7t4xb7ni%40alap3.anarazel.de Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/b779d7d8fdae088d70da5ed9fcd8205035676df3 Modified Files -------------- contrib/test_decoding/expected/sequence.out | 12 +++++------- contrib/test_decoding/sql/sequence.sql | 8 ++++---- contrib/test_decoding/test_decoding.c | 22 ++++++++++++++++++++++ 3 files changed, 31 insertions(+), 11 deletions(-)
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.) > 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 ... -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Nadie está tan esclavizado como el que se cree libre no siéndolo" (Goethe)
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
On 2/14/22 14:36, Alvaro Herrera wrote: > On 2022-Feb-13, Tomas Vondra wrote: > >>>> 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? > > Yeah, it's pointless. > > I propose the attached comment additions instead. > The comments seem fine to me (although not really related to these commits about sequence decoding). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2022-Feb-14, Tomas Vondra wrote: > On 2/14/22 14:36, Alvaro Herrera wrote: > > I propose the attached comment additions instead. > > The comments seem fine to me (although not really related to these commits > about sequence decoding). Thanks, pushed. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Now I have my system running, not a byte was off the shelf; It rarely breaks and when it does I fix the code myself. It's stable, clean and elegant, and lightning fast as well, And it doesn't cost a nickel, so Bill Gates can go to hell."