Horton

Thread

Commits

  1. Relax overly strict assertion

  2. Handle heap rewrites even better in logical decoding

  1. Too rigorous assert in reorderbuffer.c

    Arseny Sher <a.sher@postgrespro.ru> — 2019-01-31T06:21:59Z

    Hi,

My colleague Alexander Lakhin has noticed an assertion failure in
reorderbuffer.c:1330. Here is a simple snippet reproducing it:

SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');

create table t(k int);
begin;
savepoint a;
alter table t alter column k type text;
rollback to savepoint a;
alter table t alter column k type bigint;
commit;

SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');

It is indeed too opinionated since cmax of a tuple is not stable; it can
be rewritten if subxact who tried to delete it later aborts (analogy
also holds for xmax). Attached patch removes it. While here, I had also
considered worthwhile to add a test involving DDL in aborted subxact as
it is interesting anyway and wasn't covered before.

  2. Re: Too rigorous assert in reorderbuffer.c

    Alexey Kondratov <a.kondratov@postgrespro.ru> — 2019-02-04T16:15:14Z

    Hi,

On 31.01.2019 9:21, Arseny Sher wrote:
> My colleague Alexander Lakhin has noticed an assertion failure in
> reorderbuffer.c:1330. Here is a simple snippet reproducing it:
>
> SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
>
> create table t(k int);
> begin;
> savepoint a;
> alter table t alter column k type text;
> rollback to savepoint a;
> alter table t alter column k type bigint;
> commit;
>
> SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');

I just want to add, that I have accidentally discovered the same issue 
during the testing of the Tomas's large transactions streaming patch 
[1], and had to remove this assert to get things working. I thought that 
it was somehow related to the streaming mode and did not test the same 
query alone.


[1] 
https://www.postgresql.org/message-id/76fc440e-91c3-afe2-b78a-987205b3c758%402ndquadrant.com


Regards

-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

  3. Re: Too rigorous assert in reorderbuffer.c

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-05T21:11:19Z

    On 2019-Jan-31, Arseny Sher wrote:

> My colleague Alexander Lakhin has noticed an assertion failure in
> reorderbuffer.c:1330. Here is a simple snippet reproducing it:
> 
> SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
> 
> create table t(k int);
> begin;
> savepoint a;
> alter table t alter column k type text;
> rollback to savepoint a;
> alter table t alter column k type bigint;
> commit;
> 
> SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');

Hmm, the new test introduced by your patch fails in early branches (at
least 9.4): the transaction is decoded like this:

!                         data                         
! -----------------------------------------------------
   BEGIN
   table public.tr_sub_ddl: INSERT: data[integer]:42
+  table public.pg_temp_16445: INSERT: data[bigint]:42
   table public.tr_sub_ddl: INSERT: data[bigint]:43
   COMMIT
! (5 rows)

note the additional pg_temp_XYZ row in the middle.  This is caused by
the rewrite in ALTER TABLE.  Peter E fixed that in Pg11 in commit
325f2ec55; I don't think there's much to do in the backbranches other
than hide the pesky record to avoid it breaking the test.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: Too rigorous assert in reorderbuffer.c

    Arseny Sher <a.sher@postgrespro.ru> — 2019-02-06T09:21:27Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> note the additional pg_temp_XYZ row in the middle.  This is caused by
> the rewrite in ALTER TABLE.  Peter E fixed that in Pg11 in commit
> 325f2ec55; I don't think there's much to do in the backbranches other
> than hide the pesky record to avoid it breaking the test.

Oh, I see. Let's just remove the first insertion then, as in attached.
I've tested it on master and on 9.4.

--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  5. Re: Too rigorous assert in reorderbuffer.c

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-06T16:30:50Z

    On 2019-Feb-06, Arseny Sher wrote:

> 
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> 
> > note the additional pg_temp_XYZ row in the middle.  This is caused by
> > the rewrite in ALTER TABLE.  Peter E fixed that in Pg11 in commit
> > 325f2ec55; I don't think there's much to do in the backbranches other
> > than hide the pesky record to avoid it breaking the test.
> 
> Oh, I see. Let's just remove the first insertion then, as in attached.
> I've tested it on master and on 9.4.

Ah, okay.  Does the test still fail when run without the code fix?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: Too rigorous assert in reorderbuffer.c

    Arseny Sher <a.sher@postgrespro.ru> — 2019-02-07T08:50:29Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> On 2019-Feb-06, Arseny Sher wrote:
>
>>
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>
>> > note the additional pg_temp_XYZ row in the middle.  This is caused by
>> > the rewrite in ALTER TABLE.  Peter E fixed that in Pg11 in commit
>> > 325f2ec55; I don't think there's much to do in the backbranches other
>> > than hide the pesky record to avoid it breaking the test.
>>
>> Oh, I see. Let's just remove the first insertion then, as in attached.
>> I've tested it on master and on 9.4.
>
> Ah, okay.  Does the test still fail when run without the code fix?

Yes. The problem here is overriding cmax of catalog (pg_attribute in the
test) tuples, so it fails without any data at all.


--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  7. Re: Too rigorous assert in reorderbuffer.c

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-11T18:50:03Z

    On 2019-Feb-07, Arseny Sher wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> > Ah, okay.  Does the test still fail when run without the code fix?
> 
> Yes. The problem here is overriding cmax of catalog (pg_attribute in the
> test) tuples, so it fails without any data at all.

Makes sense.

I thought the blanket removal of the assert() was excessive, and we can
relax it instead; what do you think of the attached?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: Too rigorous assert in reorderbuffer.c

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-12T13:53:43Z

    On 2019-Feb-11, Alvaro Herrera wrote:

> On 2019-Feb-07, Arseny Sher wrote:
> 
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> 
> > > Ah, okay.  Does the test still fail when run without the code fix?
> > 
> > Yes. The problem here is overriding cmax of catalog (pg_attribute in the
> > test) tuples, so it fails without any data at all.
> 
> Makes sense.
> 
> I thought the blanket removal of the assert() was excessive, and we can
> relax it instead; what do you think of the attached?

More precisely, my question was: with this change, does the code still
work correctly in your non-toy case?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: Too rigorous assert in reorderbuffer.c

    Arseny Sher <a.sher@postgrespro.ru> — 2019-02-12T16:10:55Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:

>> I thought the blanket removal of the assert() was excessive, and we can
>> relax it instead; what do you think of the attached?
>
> More precisely, my question was: with this change, does the code still
> work correctly in your non-toy case?

Yes, it works. I thought for a moment that some obscure cases where cmax
on a single tuple is not strictly monotonic might exist, but looks like
they don't. So your change is ok for me, reshaping assert is better than
removing. make check is also good on all supported branches.


--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  10. Re: Too rigorous assert in reorderbuffer.c

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-12T22:00:22Z

    On 2019-Feb-12, Arseny Sher wrote:

> 
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> 
> >> I thought the blanket removal of the assert() was excessive, and we can
> >> relax it instead; what do you think of the attached?
> >
> > More precisely, my question was: with this change, does the code still
> > work correctly in your non-toy case?
> 
> Yes, it works. I thought for a moment that some obscure cases where cmax
> on a single tuple is not strictly monotonic might exist, but looks like
> they don't. So your change is ok for me, reshaping assert is better than
> removing. make check is also good on all supported branches.

Thanks for checking!  I also run it on all branches, everything passes.
Pushed now.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: Too rigorous assert in reorderbuffer.c

    Arseny Sher <a.sher@postgrespro.ru> — 2019-02-16T06:02:29Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> Thanks for checking!  I also run it on all branches, everything passes.
> Pushed now.

I'm sorry to bother you with this again, but due to new test our
internal buildfarm revealed that ajacent assert on cmin is also lie. You
see, we can't assume cmin is stable because the same key (relnode, tid)
might refer to completely different tuples if tuple was inserted by
aborted subxact, immeditaly reclaimed and then space occupied by another
one. Fix is attached.

Technically this might mean a user-facing bug, because we only pick the
first cmin which means we might get visibility wrong, allowing to see
some version too early (i.e real cmin of tuple is y, but decoding thinks
it is x, and x < y). However, I couldn't quickly make up an example
where this would actually lead to bad consequences. I tried to create
such extra visible row in pg_attribute, but that's ok because loop in
RelationBuildTupleDesc spins exactly natts times and ignores what is
left unscanned. It is also ok with pg_class, because apparently
ScanPgRelation also fishes out the (right) first tuple and doesn't check
for duplicates appearing later in the scan. Maybe I just haven't tried
hard enough though.

Attached 'aborted_subxact_test.patch' is an illustration of such wrong
cmin visibility on pg_attribute. It triggers assertion failure, but
otherwise ok (no user-facing issues), as I said earlier, so I am
disinclined to include it in the fix.

--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  12. Re: Too rigorous assert in reorderbuffer.c

    Arseny Sher <a.sher@postgrespro.ru> — 2019-12-19T16:30:10Z

    Arseny Sher <a.sher@postgrespro.ru> writes:

> I'm sorry to bother you with this again, but due to new test our
> internal buildfarm revealed that ajacent assert on cmin is also lie. You
> see, we can't assume cmin is stable because the same key (relnode, tid)
> might refer to completely different tuples if tuple was inserted by
> aborted subxact, immeditaly reclaimed and then space occupied by another
> one. Fix is attached.
>
> Technically this might mean a user-facing bug, because we only pick the
> first cmin which means we might get visibility wrong, allowing to see
> some version too early (i.e real cmin of tuple is y, but decoding thinks
> it is x, and x < y). However, I couldn't quickly make up an example
> where this would actually lead to bad consequences. I tried to create
> such extra visible row in pg_attribute, but that's ok because loop in
> RelationBuildTupleDesc spins exactly natts times and ignores what is
> left unscanned. It is also ok with pg_class, because apparently
> ScanPgRelation also fishes out the (right) first tuple and doesn't check
> for duplicates appearing later in the scan. Maybe I just haven't tried
> hard enough though.

This issue still exists, it would be nice to fix it...

--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company