Thread: Decoding of (nearly) empty transactions and regression tests

Hi,

As shown by the CLOBBER_CACHE_ALWAYS animal test_decoding's tests fail
if they take very long. The failures aren't bugs, but diffs like:  BEGIN
+  COMMIT
+  BEGIN  table public.tr_sub: INSERT: id[integer]:1 path[text]:'1-top-#1'
The added transaction is an analyze started by autovacuum.

So, I've been wondering for a while to get rid of this, but I haven't
come up with something I like.

To recap, currently the rules for visibly decoding a transaction
(i.e. calling the output plugin callbacks) are:
1) it has been WAL logged (duh)
2) it happened in the database we're decoding
3) it contains something. 'Something' currently means that a logged  table has changed, or the transaction contains
invalidations.

Note that just because a transaction contains 'something', these changes
aren't necessarily shown as we don't decode system table changes. I
think that behaviour makes sense because otherwise something like CREATE
TABLE without further changes wouldn't show up in the change stream.

Solution I)
Change ReorderBufferCommit() to not call the begin()/commit() callbacks
if no change() callback has been called. Technically that's trivial, but
I don't think that'd end up being a better behaviour.

Solution II)
Disable autovacuum/analyze for the tables in the regression tests. We
test vacuum explicitly, so we wouldn't loose too much test coverage.

Solution III)
Mark transactions that happen purely internally as such, using a
xl_xact_commit->xinfo flag. Not particularly pretty and the most
invasive of the solutions.

I'm favoring II) so far... Other opinions?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> Solution I)
> Change ReorderBufferCommit() to not call the begin()/commit() callbacks
> if no change() callback has been called. Technically that's trivial, but
> I don't think that'd end up being a better behaviour.

> Solution II)
> Disable autovacuum/analyze for the tables in the regression tests. We
> test vacuum explicitly, so we wouldn't loose too much test coverage.

> Solution III)
> Mark transactions that happen purely internally as such, using a
> xl_xact_commit->xinfo flag. Not particularly pretty and the most
> invasive of the solutions.

> I'm favoring II) so far... Other opinions?

You mean disable autovac only in the contrib/test_decoding check run,
right?  That's probably OK since it's tested in the main regression
runs.
        regards, tom lane

On 2014-06-29 10:36:22 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > Solution I)
> > Change ReorderBufferCommit() to not call the begin()/commit() callbacks
> > if no change() callback has been called. Technically that's trivial, but
> > I don't think that'd end up being a better behaviour.
> 
> > Solution II)
> > Disable autovacuum/analyze for the tables in the regression tests. We
> > test vacuum explicitly, so we wouldn't loose too much test coverage.
> 
> > Solution III)
> > Mark transactions that happen purely internally as such, using a
> > xl_xact_commit->xinfo flag. Not particularly pretty and the most
> > invasive of the solutions.
> 
> > I'm favoring II) so far... Other opinions?
> 
> You mean disable autovac only in the contrib/test_decoding check run,
> right?  That's probably OK since it's tested in the main regression
> runs.

Actually I'd not even though of that, but just of disabling it on
relations with relevant amounts of changes in the test_decoding
tests. That way it'd work even when run against an existing server (via
installcheck-force) which I found useful during development.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-06-29 10:36:22 -0400, Tom Lane wrote:
>> You mean disable autovac only in the contrib/test_decoding check run,
>> right?  That's probably OK since it's tested in the main regression
>> runs.

> Actually I'd not even though of that, but just of disabling it on
> relations with relevant amounts of changes in the test_decoding
> tests. That way it'd work even when run against an existing server (via
> installcheck-force) which I found useful during development.

I think that a change that affects the behavior in any other test runs is
not acceptable.  Our testing of autovac is weaker than I'd like already
(since the main regression runs are designed to not show visible changes
when it runs).  I don't want it being turned off elsewhere for the benefit
of this test.
        regards, tom lane

On 2014-06-29 11:08:39 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-06-29 10:36:22 -0400, Tom Lane wrote:
> >> You mean disable autovac only in the contrib/test_decoding check run,
> >> right?  That's probably OK since it's tested in the main regression
> >> runs.
> 
> > Actually I'd not even though of that, but just of disabling it on
> > relations with relevant amounts of changes in the test_decoding
> > tests. That way it'd work even when run against an existing server (via
> > installcheck-force) which I found useful during development.
> 
> I think that a change that affects the behavior in any other test runs is
> not acceptable.  Our testing of autovac is weaker than I'd like already
> (since the main regression runs are designed to not show visible changes
> when it runs).  I don't want it being turned off elsewhere for the benefit
> of this test.

Hm? I think we're misunderstanding each other - I was thinking of tacking
ALTER TABLE .. SET (AUTOVACUUM_ENABLED = false) to the tables created in
test_decoding/sql/, not to something outside.

Since we ignore transactions in other databases and the tests run in a
database freshly created by pg_regress that should get rid of spurious
transactions. Unless somebody fiddled with the template database or is
close to a wraparound - but I'd be pretty surprised if that wouldn't
cause failures in other tests as wel.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> Hm? I think we're misunderstanding each other - I was thinking of tacking
> ALTER TABLE .. SET (AUTOVACUUM_ENABLED = false) to the tables created in
> test_decoding/sql/, not to something outside.

Ah, got it.  Yeah, seems like an OK workaround.
        regards, tom lane

On 2014-06-29 11:24:35 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > Hm? I think we're misunderstanding each other - I was thinking of tacking
> > ALTER TABLE .. SET (AUTOVACUUM_ENABLED = false) to the tables created in
> > test_decoding/sql/, not to something outside.
>
> Ah, got it.  Yeah, seems like an OK workaround.

I'd tried that back then, but it didn't really work out very
well. Sometimes there's still autovacuums running on tables we don't
want to disable autovacuum on.

My new proposal (patch attached) is a skip-empty-xacts option for
test_decoding. It causes test_decoding not to output BEGIN/COMMIT for
transactions that don't output any rows.

I don't really like that approach, but it's surely better than
continually coloring the CLOBBER animal red. And the patch is, besides
the verbiage of regression test output changes, not that bad.

Unless somebody protests I'll apply that to master/9.4 later today.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services