Too rigorous assert in reorderbuffer.c
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t40050psql -h localhost -U postgresBuilt from patchset v11 (message #11), August 18, 2026 at 04:43 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t40050_11 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t40050_11 && git checkout t40050_11Patchset v11 (message #11) is on t40050_11
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.
Attachments:
0001-Remove-assertion-in-reorderbuffer-that-cmax-is-stabl.patchtext/x-diffDownload+40-5
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]: /messages/by-id/76fc440e-91c3-afe2-b78a-987205b3c758@2ndquadrant.com
it was somehow related to the streaming mode and did not test the same
query alone.
[1]: /messages/by-id/76fc440e-91c3-afe2-b78a-987205b3c758@2ndquadrant.com
/messages/by-id/76fc440e-91c3-afe2-b78a-987205b3c758@2ndquadrant.com
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
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
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
Attachments:
0001-Remove-assertion-in-reorderbuffer-that-cmax-is-stabl.patchtext/x-diffDownload+37-5
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
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
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
Attachments:
v3-0001-Remove-assertion-in-reorderbuffer-that-cmax-is-stabl.patchtext/x-diff; charset=us-asciiDownload+40-5
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
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
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
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
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