From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Arseny Sher <a.sher@postgrespro.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Alexander Lakhin <a.lakhin@postgrespro.ru>
Subject: Re: Too rigorous assert in reorderbuffer.c
Date: Mon, 11 Feb 2019 15:50:03 -0300
Message-ID: <20190211185003.GA27084@alvherre.pgsql> (raw)
In-Reply-To: <871s4k7zje.fsf@ars-thinkpad>
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.patch (text/x-diff, 3.0K) downloaddiff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.outindex 1ead37a7291..a04038555ef 100644--- a/contrib/test_decoding/expected/ddl.out+++ b/contrib/test_decoding/expected/ddl.out@@ -338,6 +338,24 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
COMMIT
(6 rows)
+-- check that DDL in aborted subtransactions handled correctly+CREATE TABLE tr_sub_ddl(data int);+BEGIN;+SAVEPOINT a;+ALTER TABLE tr_sub_ddl ALTER COLUMN data TYPE text;+INSERT INTO tr_sub_ddl VALUES ('blah-blah');+ROLLBACK TO SAVEPOINT a;+ALTER TABLE tr_sub_ddl ALTER COLUMN data TYPE bigint;+INSERT INTO tr_sub_ddl VALUES(43);+COMMIT;+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');+ data +--------------------------------------------------+ BEGIN+ table public.tr_sub_ddl: INSERT: data[bigint]:43+ COMMIT+(3 rows)+
/*
* Check whether treating a table as a catalog table works somewhat
*/
diff --git a/contrib/test_decoding/sql/ddl.sql b/contrib/test_decoding/sql/ddl.sqlindex 7f3446e70d4..ae9835fe869 100644--- a/contrib/test_decoding/sql/ddl.sql+++ b/contrib/test_decoding/sql/ddl.sql@@ -215,6 +215,19 @@ INSERT INTO tr_sub(path) VALUES ('5-top-1-#1');
COMMIT;
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');++-- check that DDL in aborted subtransactions handled correctly+CREATE TABLE tr_sub_ddl(data int);+BEGIN;+SAVEPOINT a;+ALTER TABLE tr_sub_ddl ALTER COLUMN data TYPE text;+INSERT INTO tr_sub_ddl VALUES ('blah-blah');+ROLLBACK TO SAVEPOINT a;+ALTER TABLE tr_sub_ddl ALTER COLUMN data TYPE bigint;+INSERT INTO tr_sub_ddl VALUES(43);+COMMIT;+
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.cindex 42d44816241..fbc2b6fe1ee 100644--- a/src/backend/replication/logical/reorderbuffer.c+++ b/src/backend/replication/logical/reorderbuffer.c@@ -1326,15 +1326,19 @@ ReorderBufferBuildTupleCidHash(ReorderBuffer *rb, ReorderBufferTXN *txn)
}
else
{
+ /*+ * Maybe we already saw this tuple before in this transaction,+ * but if so it must have the same cmin.+ */
Assert(ent->cmin == change->data.tuplecid.cmin);
- Assert(ent->cmax == InvalidCommandId ||- ent->cmax == change->data.tuplecid.cmax);
/*
- * if the tuple got valid in this transaction and now got deleted- * we already have a valid cmin stored. The cmax will be- * InvalidCommandId though.+ * cmax may be initially invalid, but once set it can only grow,+ * and never become invalid again.
*/
+ Assert((ent->cmax == InvalidCommandId) ||+ ((change->data.tuplecid.cmax != InvalidCommandId) &&+ (change->data.tuplecid.cmax > ent->cmax)));
ent->cmax = change->data.tuplecid.cmax;
}
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, a.sher@postgrespro.ru, a.lakhin@postgrespro.ru
Subject: Re: Too rigorous assert in reorderbuffer.c
In-Reply-To: <20190211185003.GA27084@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox