pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedDump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
21+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2024-03-08 270af6f0df Admit deferrable PKs into rd_pkindex, but flag them as such
* Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2023-09-20 12:58 Amul Sul <sulamul@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Amul Sul @ 2023-09-20 12:58 UTC (permalink / raw)
To: pgsql-hackers
Hi,
On the latest master head, I can see a $subject bug that seems to be related
commit #b0e96f311985:
Here is the table definition:
create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
And after restore from the dump, it shows a descriptor where column 'i' not
marked NOT NULL:
=# \d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
i | integer | | |
j | integer | | |
Indexes:
"pk" PRIMARY KEY, btree (i) DEFERRABLE
The pg_attribute entry:
=# select attname, attnotnull from pg_attribute
where attrelid = 'foo'::regclass and attnum > 0;
attname | attnotnull
---------+------------
i | f
j | f
(2 rows)
--
Regards,
Amul Sul
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2023-09-20 14:59 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Amul Sul <sulamul@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2023-09-20 14:59 UTC (permalink / raw)
To: Amul Sul <sulamul@gmail.com>; +Cc: pgsql-hackers
On 2023-Sep-20, Amul Sul wrote:
> On the latest master head, I can see a $subject bug that seems to be related
> commit #b0e96f311985:
>
> Here is the table definition:
> create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
Interesting, thanks for the report. Your attribution to that commit is
correct. The table is dumped like this:
CREATE TABLE public.foo (
i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
j integer
);
ALTER TABLE ONLY public.foo
ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
so the problem here is that the deferrable PK is not considered a reason
to keep attnotnull set, so we produce a throwaway constraint that we
then drop. This is already bogus, but what is more bogus is the fact
that the backend accepts the DROP CONSTRAINT at all.
The pg_dump failing should be easy to fix, but fixing the backend to
error out sounds more critical. So, the reason for this behavior is
that RelationGetIndexList doesn't want to list an index that isn't
marked indimmediate as a primary key. I can easily hack around that by
doing
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 7234cb3da6..971d9c8738 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
* check them.
*/
if (!index->indisunique ||
- !index->indimmediate ||
!heap_attisnull(htup, Anum_pg_index_indpred, NULL))
continue;
@@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
pkeyIndex = index->indexrelid;
+ if (!index->indimmediate)
+ continue;
+
if (!index->indisvalid)
continue;
But of course this is not great, since it impacts unrelated bits of code
that are relying on relation->pkindex or RelationGetIndexAttrBitmap
having their current behavior with non-immediate index.
I think a real solution is to stop relying on RelationGetIndexAttrBitmap
in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
to avoid printing a throwaway NOT NULL constraint at this point.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2023-09-22 10:55 Amul Sul <sulamul@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 21+ messages in thread
From: Amul Sul @ 2023-09-22 10:55 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers
On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
> On 2023-Sep-20, Amul Sul wrote:
>
> > On the latest master head, I can see a $subject bug that seems to be
> related
> > commit #b0e96f311985:
> >
> > Here is the table definition:
> > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
>
> Interesting, thanks for the report. Your attribution to that commit is
> correct. The table is dumped like this:
>
> CREATE TABLE public.foo (
> i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
> j integer
> );
> ALTER TABLE ONLY public.foo
> ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
> ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
>
> so the problem here is that the deferrable PK is not considered a reason
> to keep attnotnull set, so we produce a throwaway constraint that we
> then drop. This is already bogus, but what is more bogus is the fact
> that the backend accepts the DROP CONSTRAINT at all.
>
> The pg_dump failing should be easy to fix, but fixing the backend to
> error out sounds more critical. So, the reason for this behavior is
> that RelationGetIndexList doesn't want to list an index that isn't
> marked indimmediate as a primary key. I can easily hack around that by
> doing
>
> diff --git a/src/backend/utils/cache/relcache.c
> b/src/backend/utils/cache/relcache.c
> index 7234cb3da6..971d9c8738 100644
> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
> * check them.
> */
> if (!index->indisunique ||
> - !index->indimmediate ||
> !heap_attisnull(htup,
> Anum_pg_index_indpred, NULL))
> continue;
>
> @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
> relation->rd_rel->relkind ==
> RELKIND_PARTITIONED_TABLE))
> pkeyIndex = index->indexrelid;
>
> + if (!index->indimmediate)
> + continue;
> +
> if (!index->indisvalid)
> continue;
>
>
> But of course this is not great, since it impacts unrelated bits of code
> that are relying on relation->pkindex or RelationGetIndexAttrBitmap
> having their current behavior with non-immediate index.
>
True, but still wondering why would relation->rd_pkattr skipped for a
deferrable primary key, which seems to be a bit incorrect to me since it
sensing that relation doesn't have PK at all. Anyway, that is unrelated.
> I think a real solution is to stop relying on RelationGetIndexAttrBitmap
> in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
> to avoid printing a throwaway NOT NULL constraint at this point.)
>
I might not have understood this, but I think, if it is ok to skip
throwaway NOT
NULL for deferrable PK then that would be enough for the reported issue
to be fixed. I quickly tried with the attached patch which looks sufficient
to skip that, but, TBH, I haven't thought carefully about this change.
Regards,
Amul
Attachments:
trial_skip_throwaway_non_null.patch (application/x-patch, 622B) download
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f7b61766921..5a4e915dc62 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8543,7 +8543,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
appendPQExpBufferStr(q,
"LEFT JOIN pg_catalog.pg_constraint copk ON "
"(copk.conrelid = src.tbloid\n"
- " AND copk.contype = 'p' AND "
+ " AND copk.contype = 'p' AND copk.condeferrable = false AND "
"copk.conkey @> array[a.attnum])\n"
"WHERE a.attnum > 0::pg_catalog.int2\n"
"ORDER BY a.attrelid, a.attnum");
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-01-20 02:24 vignesh C <vignesh21@gmail.com>
parent: Amul Sul <sulamul@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: vignesh C @ 2024-01-20 02:24 UTC (permalink / raw)
To: Amul Sul <sulamul@gmail.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers
On Fri, 22 Sept 2023 at 18:45, Amul Sul <sulamul@gmail.com> wrote:
>
>
>
> On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>
>> On 2023-Sep-20, Amul Sul wrote:
>>
>> > On the latest master head, I can see a $subject bug that seems to be related
>> > commit #b0e96f311985:
>> >
>> > Here is the table definition:
>> > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
>>
>> Interesting, thanks for the report. Your attribution to that commit is
>> correct. The table is dumped like this:
>>
>> CREATE TABLE public.foo (
>> i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
>> j integer
>> );
>> ALTER TABLE ONLY public.foo
>> ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
>> ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
>>
>> so the problem here is that the deferrable PK is not considered a reason
>> to keep attnotnull set, so we produce a throwaway constraint that we
>> then drop. This is already bogus, but what is more bogus is the fact
>> that the backend accepts the DROP CONSTRAINT at all.
>>
>> The pg_dump failing should be easy to fix, but fixing the backend to
>> error out sounds more critical. So, the reason for this behavior is
>> that RelationGetIndexList doesn't want to list an index that isn't
>> marked indimmediate as a primary key. I can easily hack around that by
>> doing
>>
>> diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
>> index 7234cb3da6..971d9c8738 100644
>> --- a/src/backend/utils/cache/relcache.c
>> +++ b/src/backend/utils/cache/relcache.c
>> @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
>> * check them.
>> */
>> if (!index->indisunique ||
>> - !index->indimmediate ||
>> !heap_attisnull(htup, Anum_pg_index_indpred, NULL))
>> continue;
>>
>> @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
>> relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
>> pkeyIndex = index->indexrelid;
>>
>> + if (!index->indimmediate)
>> + continue;
>> +
>> if (!index->indisvalid)
>> continue;
>>
>>
>> But of course this is not great, since it impacts unrelated bits of code
>> that are relying on relation->pkindex or RelationGetIndexAttrBitmap
>> having their current behavior with non-immediate index.
>
>
> True, but still wondering why would relation->rd_pkattr skipped for a
> deferrable primary key, which seems to be a bit incorrect to me since it
> sensing that relation doesn't have PK at all. Anyway, that is unrelated.
>
>>
>> I think a real solution is to stop relying on RelationGetIndexAttrBitmap
>> in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
>> to avoid printing a throwaway NOT NULL constraint at this point.)
>
>
> I might not have understood this, but I think, if it is ok to skip throwaway NOT
> NULL for deferrable PK then that would be enough for the reported issue
> to be fixed. I quickly tried with the attached patch which looks sufficient
> to skip that, but, TBH, I haven't thought carefully about this change.
I did not see any test addition for this, can we add it?
Regards,
Vignesh
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-01-23 12:11 Amul Sul <sulamul@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Amul Sul @ 2024-01-23 12:11 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers
On Sat, Jan 20, 2024 at 7:55 AM vignesh C <vignesh21@gmail.com> wrote:
> On Fri, 22 Sept 2023 at 18:45, Amul Sul <sulamul@gmail.com> wrote:
> >
> >
> >
> > On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> >>
> >> On 2023-Sep-20, Amul Sul wrote:
> >>
> >> > On the latest master head, I can see a $subject bug that seems to be
> related
> >> > commit #b0e96f311985:
> >> >
> >> > Here is the table definition:
> >> > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i)
> DEFERRABLE);
> >>
> >> Interesting, thanks for the report. Your attribution to that commit is
> >> correct. The table is dumped like this:
> >>
> >> CREATE TABLE public.foo (
> >> i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
> >> j integer
> >> );
> >> ALTER TABLE ONLY public.foo
> >> ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
> >> ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
> >>
> >> so the problem here is that the deferrable PK is not considered a reason
> >> to keep attnotnull set, so we produce a throwaway constraint that we
> >> then drop. This is already bogus, but what is more bogus is the fact
> >> that the backend accepts the DROP CONSTRAINT at all.
> >>
> >> The pg_dump failing should be easy to fix, but fixing the backend to
> >> error out sounds more critical. So, the reason for this behavior is
> >> that RelationGetIndexList doesn't want to list an index that isn't
> >> marked indimmediate as a primary key. I can easily hack around that by
> >> doing
> >>
> >> diff --git a/src/backend/utils/cache/relcache.c
> b/src/backend/utils/cache/relcache.c
> >> index 7234cb3da6..971d9c8738 100644
> >> --- a/src/backend/utils/cache/relcache.c
> >> +++ b/src/backend/utils/cache/relcache.c
> >> @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
> >> * check them.
> >> */
> >> if (!index->indisunique ||
> >> - !index->indimmediate ||
> >> !heap_attisnull(htup,
> Anum_pg_index_indpred, NULL))
> >> continue;
> >>
> >> @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
> >> relation->rd_rel->relkind ==
> RELKIND_PARTITIONED_TABLE))
> >> pkeyIndex = index->indexrelid;
> >>
> >> + if (!index->indimmediate)
> >> + continue;
> >> +
> >> if (!index->indisvalid)
> >> continue;
> >>
> >>
> >> But of course this is not great, since it impacts unrelated bits of code
> >> that are relying on relation->pkindex or RelationGetIndexAttrBitmap
> >> having their current behavior with non-immediate index.
> >
> >
> > True, but still wondering why would relation->rd_pkattr skipped for a
> > deferrable primary key, which seems to be a bit incorrect to me since it
> > sensing that relation doesn't have PK at all. Anyway, that is unrelated.
> >
> >>
> >> I think a real solution is to stop relying on RelationGetIndexAttrBitmap
> >> in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
> >> to avoid printing a throwaway NOT NULL constraint at this point.)
> >
> >
> > I might not have understood this, but I think, if it is ok to skip
> throwaway NOT
> > NULL for deferrable PK then that would be enough for the reported issue
> > to be fixed. I quickly tried with the attached patch which looks
> sufficient
> > to skip that, but, TBH, I haven't thought carefully about this change.
>
> I did not see any test addition for this, can we add it?
>
Ok, added it in the attached version.
This was an experimental patch, I was looking for the comment on the
proposed
approach -- whether we could simply skip the throwaway NOT NULL constraint
for
deferred PK constraint. Moreover, skip that for any PK constraint.
Regards,
Amul
Attachments:
trial_skip_throwaway_non_null_v2.patch (application/octet-stream, 1.4K) download
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bc20a025ce..c30eb95ce4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8740,7 +8740,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
appendPQExpBufferStr(q,
"LEFT JOIN pg_catalog.pg_constraint copk ON "
"(copk.conrelid = src.tbloid\n"
- " AND copk.contype = 'p' AND "
+ " AND copk.contype = 'p' AND copk.condeferrable = false AND "
"copk.conkey @> array[a.attnum])\n"
"WHERE a.attnum > 0::pg_catalog.int2\n"
"ORDER BY a.attrelid, a.attnum");
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 3912dbf481..43913e3476 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3605,12 +3605,12 @@ my %tests = (
'CREATE TABLE test_table_generated' => {
create_order => 3,
create_sql => 'CREATE TABLE dump_test.test_table_generated (
- col1 int primary key,
+ col1 int primary key deferrable,
col2 int generated always as (col1 * 2) stored
);',
regexp => qr/^
\QCREATE TABLE dump_test.test_table_generated (\E\n
- \s+\Qcol1 integer CONSTRAINT \E[a-z0-9_]*\Q NOT NULL NO INHERIT,\E\n
+ \s+\Qcol1 integer,\E\n
\s+\Qcol2 integer GENERATED ALWAYS AS ((col1 * 2)) STORED\E\n
\);
/xms,
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-04 12:34 Aleksander Alekseev <aleksander@timescale.com>
parent: Amul Sul <sulamul@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Aleksander Alekseev @ 2024-03-04 12:34 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>; Alvaro Herrera <alvherre@alvh.no-ip.org>
Hi hackers,
>> I did not see any test addition for this, can we add it?
>
>
> Ok, added it in the attached version.
>
> This was an experimental patch, I was looking for the comment on the proposed
> approach -- whether we could simply skip the throwaway NOT NULL constraint for
> deferred PK constraint. Moreover, skip that for any PK constraint.
I confirm that the patch fixes the bug. All the tests pass. Looks like
RfC to me.
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-04 13:50 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Aleksander Alekseev <aleksander@timescale.com>
0 siblings, 1 reply; 21+ messages in thread
From: Dean Rasheed @ 2024-03-04 13:50 UTC (permalink / raw)
To: Aleksander Alekseev <aleksander@timescale.com>; +Cc: pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>; Alvaro Herrera <alvherre@alvh.no-ip.org>
On Mon, 4 Mar 2024 at 12:34, Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> > This was an experimental patch, I was looking for the comment on the proposed
> > approach -- whether we could simply skip the throwaway NOT NULL constraint for
> > deferred PK constraint. Moreover, skip that for any PK constraint.
>
> I confirm that the patch fixes the bug. All the tests pass. Looks like
> RfC to me.
>
I don't think that this is the right fix. ISTM that the real issue is
that dropping a NOT NULL constraint should not mark the column as
nullable if it is part of a PK, whether or not that PK is deferrable
-- a deferrable PK still marks a column as not nullable.
The reason pg_dump creates these throwaway NOT NULL constraints is to
avoid a table scan to check for NULLs when the PK is later created.
That rationale still applies to deferrable PKs, so we still want the
throwaway NOT NULL constraints in that case, otherwise we'd be hurting
performance of restore.
Regards,
Dean
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-05 12:36 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2024-03-05 12:36 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On 2024-Mar-04, Dean Rasheed wrote:
> I don't think that this is the right fix. ISTM that the real issue is
> that dropping a NOT NULL constraint should not mark the column as
> nullable if it is part of a PK, whether or not that PK is deferrable
> -- a deferrable PK still marks a column as not nullable.
Yeah. As I said upthread, a good fix seems to require no longer relying
on RelationGetIndexAttrBitmap to obtain the columns in the primary key,
because that function does not include deferred primary keys. I came up
with the attached POC, which seems to fix the reported problem, but of
course it needs more polish, a working test case, and verifying whether
the new function should be used in more places -- in particular, whether
it can be used to revert the changes to RelationGetIndexList that
b0e96f311985 did.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
en duras y empinadas sillas / desprovistas, por cierto
de blandos atenuantes" (Patricio Vogel)
Attachments:
0001-Don-t-lose-attnotnull-if-a-deferred-PK-supports-it.patch (text/x-diff, 3.3K) download
From cc7032c3f62949d20bc98d4b3c70cb2cb8a0dd5e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 5 Mar 2024 13:32:50 +0100
Subject: [PATCH] Don't lose attnotnull if a deferred PK supports it
When dropping a NOT NULL constraint on a column that has a deferred
primary key, we would reset attnotnull, but that's bogus.
XXX this patch is nowhere near final.
Reported-by: Amul Sul <sulamul@gmail.com>
Discussion: https://postgr.es/m/CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com
---
src/backend/commands/tablecmds.c | 71 ++++++++++++++++++++++++++++++--
1 file changed, 68 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c61f9305c2..84c7871dda 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12704,6 +12704,73 @@ ATExecDropConstraint(Relation rel, const char *constrName,
table_close(conrel, RowExclusiveLock);
}
+/*
+ * dropconstraint_getpkcols -- subroutine for dropconstraint_internal
+ *
+ * This is a workaround to the fact that RelationGetIndexAttrBitmap does
+ * not consider a DEFERRABLE PRIMARY KEY to be a real primary key. Maybe
+ * this code should be elsewhere.
+ */
+static Bitmapset *
+dropconstraint_getpkcols(Relation rel)
+{
+ Relation indrel;
+ SysScanDesc indscan;
+ ScanKeyData skey;
+ HeapTuple htup;
+ Bitmapset *b = NULL;
+
+ /*
+ * We try first to obtain a list of PK columns from the cache; if there
+ * is one, we're done.
+ */
+ b = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_PRIMARY_KEY);
+ if (b != NULL)
+ return b;
+
+ /* Prepare to scan pg_index for entries having indrelid = this rel. */
+ ScanKeyInit(&skey,
+ Anum_pg_index_indrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(RelationGetRelid(rel)));
+
+ indrel = table_open(IndexRelationId, AccessShareLock);
+ indscan = systable_beginscan(indrel, IndexIndrelidIndexId, true,
+ NULL, 1, &skey);
+
+ while (HeapTupleIsValid(htup = systable_getnext(indscan)))
+ {
+ Form_pg_index index = (Form_pg_index) GETSTRUCT(htup);
+
+ /*
+ * Ignore any indexes that are currently being dropped and those that
+ * aren't a primary key.
+ */
+ if (!index->indislive)
+ continue;
+ if (!index->indisprimary)
+ continue;
+
+ /*
+ * If this primary key was IMMEDIATE, it would have been returned
+ * by RelationGetIndexAttrBitmap.
+ */
+ Assert(!index->indimmediate);
+
+ for (int i = 0; i < index->indnatts; i++)
+ {
+ int attrnum = index->indkey.values[i];
+
+ b = bms_add_member(b, attrnum - FirstLowInvalidHeapAttributeNumber);
+ }
+ }
+
+ systable_endscan(indscan);
+ table_close(indrel, AccessShareLock);
+
+ return b;
+}
+
/*
* Remove a constraint, using its pg_constraint tuple
*
@@ -12837,9 +12904,7 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha
* We want to test columns for their presence in the primary key, but
* only if we're not dropping it.
*/
- pkcols = dropping_pk ? NULL :
- RelationGetIndexAttrBitmap(rel,
- INDEX_ATTR_BITMAP_PRIMARY_KEY);
+ pkcols = dropping_pk ? NULL : dropconstraint_getpkcols(rel);
ircols = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY);
foreach(lc, unconstrained_cols)
--
2.39.2
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-05 14:58 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 2 replies; 21+ messages in thread
From: Dean Rasheed @ 2024-03-05 14:58 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On Tue, 5 Mar 2024 at 12:36, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Yeah. As I said upthread, a good fix seems to require no longer relying
> on RelationGetIndexAttrBitmap to obtain the columns in the primary key,
> because that function does not include deferred primary keys. I came up
> with the attached POC, which seems to fix the reported problem, but of
> course it needs more polish, a working test case, and verifying whether
> the new function should be used in more places -- in particular, whether
> it can be used to revert the changes to RelationGetIndexList that
> b0e96f311985 did.
>
Looking at the other places that call RelationGetIndexAttrBitmap()
with INDEX_ATTR_BITMAP_PRIMARY_KEY, they all appear to want to include
deferrable PKs, since they are relying on the result to see which
columns are not nullable.
So there are other bugs here. For example:
CREATE TABLE foo (id int PRIMARY KEY DEFERRABLE, val text);
CREATE TABLE bar (LIKE foo);
now fails to mark bar.id as not nullable, whereas prior to
b0e96f311985 it would have been.
So I think RelationGetIndexAttrBitmap() should include deferrable PKs,
but not all the changes made to RelationGetIndexList() by b0e96f311985
need reverting.
Regards,
Dean
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-05 17:18 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
1 sibling, 0 replies; 21+ messages in thread
From: Alvaro Herrera @ 2024-03-05 17:18 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On 2024-Mar-05, Dean Rasheed wrote:
> Looking at the other places that call RelationGetIndexAttrBitmap()
> with INDEX_ATTR_BITMAP_PRIMARY_KEY, they all appear to want to include
> deferrable PKs, since they are relying on the result to see which
> columns are not nullable.
Hmm, I find this pretty surprising, but you are right. Somehow I had
the idea that INDEX_ATTR_BITMAP_PRIMARY_KEY was used for planning
activities so I didn't want to modify its behavior ... but clearly
that's not at all the case. It's only used for DDL, and one check in
logical replication.
> So there are other bugs here. For example:
>
> CREATE TABLE foo (id int PRIMARY KEY DEFERRABLE, val text);
> CREATE TABLE bar (LIKE foo);
>
> now fails to mark bar.id as not nullable, whereas prior to
> b0e96f311985 it would have been.
Fun. (Thankfully, easy to fix. But I'll add this as a test too.)
> So I think RelationGetIndexAttrBitmap() should include deferrable PKs,
Yeah, I'll go make it so. I think I'll add a test for the case that
changes behavior in logical replication first (which is that the target
relation of logical replication is currently not marked as updatable,
when its PK is deferrable).
> but not all the changes made to RelationGetIndexList() by b0e96f311985
> need reverting.
I'll give this a look too.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-07 13:00 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
1 sibling, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2024-03-07 13:00 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On 2024-Mar-05, Dean Rasheed wrote:
> So I think RelationGetIndexAttrBitmap() should include deferrable PKs,
I tried this, but it doesn't actually lead to a good place, because if
we allow deferrable PKs to identify rows, then they are not useful to
find the tuple to update when replicating. Consider the following case:
$node_publisher->safe_psql('postgres',
'create table deferred_pk (id int primary key initially deferred, hidden int, value text)');
$node_subscriber->safe_psql('postgres',
'create table deferred_pk (id int primary key initially deferred, hidden int, value text)');
$node_subscriber->safe_psql('postgres',
'alter subscription tap_sub refresh publication');
$node_publisher->safe_psql('postgres',
"insert into deferred_pk (id, hidden, value) values (1, 1, 'first')");
$node_publisher->wait_for_catchup('tap_sub');
$node_publisher->safe_psql('postgres',
qq{
begin;
insert into deferred_pk values (1, 2, 'conflicting');
update deferred_pk set value = value || ', updated' where id = 1 and hidden = 2;
update deferred_pk set id = 3, value = value || ', updated' where hidden = 2;
commit});
$node_publisher->wait_for_catchup('tap_sub');
my $pubdata = $node_publisher->safe_psql('postgres',
'select * from deferred_pk order by id');
my $subsdata = $node_subscriber->safe_psql('postgres',
'select * from deferred_pk order by id');
is($subsdata, $pubdata, "data is equal");
Here, the publisher's transaction first creates a new record with the
same PK, which only works because the PK is deferred; then we update its
payload column. When this is replicated, the row is identified by the
PK ... but replication actually updates the other row, because it's
found first:
# Failed test 'data is equal'
# at t/003_constraints.pl line 163.
# got: '1|2|conflicting
# 3|2|conflicting, updated, updated'
# expected: '1|1|first
# 3|2|conflicting, updated, updated'
Actually, is that what happened here? I'm not sure, but clearly this is
bogus.
So I think the original developers of REPLICA IDENTITY had the right
idea here (commit 07cacba983ef), and we mustn't change this aspect,
because it'll lead to data corruption in replication. Using a deferred
PK for DDL considerations seems OK, but it seems certain that for actual
data replication it's going to be a disaster.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-07 13:42 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 21+ messages in thread
From: Dean Rasheed @ 2024-03-07 13:42 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On Thu, 7 Mar 2024 at 13:00, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> So I think the original developers of REPLICA IDENTITY had the right
> idea here (commit 07cacba983ef), and we mustn't change this aspect,
> because it'll lead to data corruption in replication. Using a deferred
> PK for DDL considerations seems OK, but it seems certain that for actual
> data replication it's going to be a disaster.
>
Yes, that makes sense. If I understand correctly though, the
replication code uses relation->rd_replidindex (not
relation->rd_pkindex), although sometimes it's the same thing. So can
we get away with making sure that RelationGetIndexList() doesn't set
relation->rd_replidindex to a deferrable PK, while still allowing
relation->rd_pkindex to be one?
Regards,
Dean
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-07 15:10 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2024-03-07 15:10 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On 2024-Mar-07, Dean Rasheed wrote:
> On Thu, 7 Mar 2024 at 13:00, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > So I think the original developers of REPLICA IDENTITY had the right
> > idea here (commit 07cacba983ef), and we mustn't change this aspect,
> > because it'll lead to data corruption in replication. Using a deferred
> > PK for DDL considerations seems OK, but it seems certain that for actual
> > data replication it's going to be a disaster.
>
> Yes, that makes sense. If I understand correctly though, the
> replication code uses relation->rd_replidindex (not
> relation->rd_pkindex), although sometimes it's the same thing. So can
> we get away with making sure that RelationGetIndexList() doesn't set
> relation->rd_replidindex to a deferrable PK, while still allowing
> relation->rd_pkindex to be one?
Well, not really, because the logical replication code for some reason
uses GetRelationIdentityOrPK(), which falls back to rd->pk_index (via
RelationGetPrimaryKeyIndex) if rd_replindex is not set.
Maybe we can add a flag RelationData->rd_ispkdeferred, so that
RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
logical replication would continue to not know about this PK, which
perhaps is what we want. I'll do some testing with this.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-07 17:32 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 3 replies; 21+ messages in thread
From: Alvaro Herrera @ 2024-03-07 17:32 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On 2024-Mar-07, Alvaro Herrera wrote:
> Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> logical replication would continue to not know about this PK, which
> perhaps is what we want. I'll do some testing with this.
This seems to work okay.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)
Attachments:
0001-Admit-deferrable-PKs-into-rd_pkindex-but-flag-them.patch (text/x-diff, 9.0K) download
From 8e3f913ca6a5bdbdbccac8d750ee9a6b6889f9aa Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 7 Mar 2024 16:44:35 +0100
Subject: [PATCH] Admit deferrable PKs into rd_pkindex, but flag them
... and don't return them as replica identity.
---
src/backend/utils/cache/relcache.c | 26 ++++++++++++++-----
src/include/utils/rel.h | 1 +
src/test/regress/expected/publication.out | 10 +++++++
.../regress/expected/replica_identity.out | 5 ++++
src/test/regress/sql/publication.sql | 9 +++++++
src/test/regress/sql/replica_identity.sql | 4 +++
6 files changed, 48 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 4d339ee795..0460796cf5 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4761,6 +4761,7 @@ RelationGetIndexList(Relation relation)
char replident = relation->rd_rel->relreplident;
Oid pkeyIndex = InvalidOid;
Oid candidateIndex = InvalidOid;
+ bool pkdeferred = false;
MemoryContext oldcxt;
/* Quick exit if we already computed the list. */
@@ -4802,12 +4803,12 @@ RelationGetIndexList(Relation relation)
result = lappend_oid(result, index->indexrelid);
/*
- * Non-unique, non-immediate or predicate indexes aren't interesting
- * for either oid indexes or replication identity indexes, so don't
- * check them.
+ * Non-unique or predicate indexes aren't interesting for either oid
+ * indexes or replication identity indexes, so don't check them.
+ * Deferred ones are not useful for replication identity either; but
+ * we do include them if they are PKs.
*/
if (!index->indisunique ||
- !index->indimmediate ||
!heap_attisnull(htup, Anum_pg_index_indpred, NULL))
continue;
@@ -4832,7 +4833,13 @@ RelationGetIndexList(Relation relation)
if (index->indisprimary &&
(index->indisvalid ||
relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
+ {
pkeyIndex = index->indexrelid;
+ pkdeferred = !index->indimmediate;
+ }
+
+ if (!index->indimmediate)
+ continue;
if (!index->indisvalid)
continue;
@@ -4854,7 +4861,8 @@ RelationGetIndexList(Relation relation)
oldlist = relation->rd_indexlist;
relation->rd_indexlist = list_copy(result);
relation->rd_pkindex = pkeyIndex;
- if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex))
+ relation->rd_ispkdeferred = pkdeferred;
+ if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex) && !pkdeferred)
relation->rd_replidindex = pkeyIndex;
else if (replident == REPLICA_IDENTITY_INDEX && OidIsValid(candidateIndex))
relation->rd_replidindex = candidateIndex;
@@ -4957,7 +4965,8 @@ RelationGetStatExtList(Relation relation)
/*
* RelationGetPrimaryKeyIndex -- get OID of the relation's primary key index
*
- * Returns InvalidOid if there is no such index.
+ * Returns InvalidOid if there is no such index, or if the primary key is
+ * DEFERRABLE.
*/
Oid
RelationGetPrimaryKeyIndex(Relation relation)
@@ -4972,7 +4981,7 @@ RelationGetPrimaryKeyIndex(Relation relation)
Assert(relation->rd_indexvalid);
}
- return relation->rd_pkindex;
+ return relation->rd_ispkdeferred ? InvalidOid : relation->rd_pkindex;
}
/*
@@ -5198,6 +5207,9 @@ RelationGetIndexPredicate(Relation relation)
* Attribute numbers are offset by FirstLowInvalidHeapAttributeNumber so that
* we can include system attributes (e.g., OID) in the bitmap representation.
*
+ * Deferred indexes are considered for the primary key, but not for replica
+ * identity.
+ *
* Caller had better hold at least RowExclusiveLock on the target relation
* to ensure it is safe (deadlock-free) for us to take locks on the relation's
* indexes. Note that since the introduction of CREATE INDEX CONCURRENTLY,
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index b3ea2b2042..51f876b5b5 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -63,6 +63,7 @@ typedef struct RelationData
bool rd_isvalid; /* relcache entry is valid */
bool rd_indexvalid; /* is rd_indexlist valid? (also rd_pkindex and
* rd_replidindex) */
+ bool rd_ispkdeferred;
bool rd_statvalid; /* is rd_statlist valid? */
/*----------
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 16361a91f9..0c5521d2aa 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -732,6 +732,16 @@ ALTER PUBLICATION testpub_table_ins ADD TABLE testpub_tbl5 (a); -- ok
Tables:
"public.testpub_tbl5" (a)
+-- error: cannot work with deferrable primary keys
+CREATE TABLE testpub_tbl5d (a int PRIMARY KEY DEFERRABLE);
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5d;
+UPDATE testpub_tbl5d SET a = 1;
+ERROR: cannot update table "testpub_tbl5d" because it does not have a replica identity and publishes updates
+HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+/* but works fine with FULL replica identity */
+ALTER TABLE testpub_tbl5d REPLICA IDENTITY FULL;
+UPDATE testpub_tbl5d SET a = 1;
+DROP TABLE testpub_tbl5d;
-- tests with REPLICA IDENTITY FULL
CREATE TABLE testpub_tbl6 (a int, b text, c text);
ALTER TABLE testpub_tbl6 REPLICA IDENTITY FULL;
diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out
index 6038bf8e9f..cb3ef599d5 100644
--- a/src/test/regress/expected/replica_identity.out
+++ b/src/test/regress/expected/replica_identity.out
@@ -7,6 +7,7 @@ CREATE TABLE test_replica_identity (
CONSTRAINT test_replica_identity_unique_nondefer UNIQUE (keya, keyb)
) ;
CREATE TABLE test_replica_identity_othertable (id serial primary key);
+CREATE TABLE test_replica_identity_t3 (id serial constraint pk primary key deferrable);
CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
CREATE UNIQUE INDEX test_replica_identity_nonkey ON test_replica_identity (keya, nonkey);
@@ -57,6 +58,9 @@ ERROR: "test_replica_identity_othertable_pkey" is not an index for table "test_
-- fail, deferrable
ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_unique_defer;
ERROR: cannot use non-immediate index "test_replica_identity_unique_defer" as replica identity
+-- fail, deferrable
+ALTER TABLE test_replica_identity_t3 REPLICA IDENTITY USING INDEX pk;
+ERROR: cannot use non-immediate index "pk" as replica identity
SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity'::regclass;
relreplident
--------------
@@ -292,3 +296,4 @@ DROP TABLE test_replica_identity3;
DROP TABLE test_replica_identity4;
DROP TABLE test_replica_identity5;
DROP TABLE test_replica_identity_othertable;
+DROP TABLE test_replica_identity_t3;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index d5051a5e74..8ba8036bfb 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -444,6 +444,15 @@ RESET client_min_messages;
ALTER PUBLICATION testpub_table_ins ADD TABLE testpub_tbl5 (a); -- ok
\dRp+ testpub_table_ins
+-- error: cannot work with deferrable primary keys
+CREATE TABLE testpub_tbl5d (a int PRIMARY KEY DEFERRABLE);
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5d;
+UPDATE testpub_tbl5d SET a = 1;
+/* but works fine with FULL replica identity */
+ALTER TABLE testpub_tbl5d REPLICA IDENTITY FULL;
+UPDATE testpub_tbl5d SET a = 1;
+DROP TABLE testpub_tbl5d;
+
-- tests with REPLICA IDENTITY FULL
CREATE TABLE testpub_tbl6 (a int, b text, c text);
ALTER TABLE testpub_tbl6 REPLICA IDENTITY FULL;
diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql
index dd43650586..30daec05b7 100644
--- a/src/test/regress/sql/replica_identity.sql
+++ b/src/test/regress/sql/replica_identity.sql
@@ -8,6 +8,7 @@ CREATE TABLE test_replica_identity (
) ;
CREATE TABLE test_replica_identity_othertable (id serial primary key);
+CREATE TABLE test_replica_identity_t3 (id serial constraint pk primary key deferrable);
CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
@@ -40,6 +41,8 @@ ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_iden
ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_othertable_pkey;
-- fail, deferrable
ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_unique_defer;
+-- fail, deferrable
+ALTER TABLE test_replica_identity_t3 REPLICA IDENTITY USING INDEX pk;
SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity'::regclass;
@@ -137,3 +140,4 @@ DROP TABLE test_replica_identity3;
DROP TABLE test_replica_identity4;
DROP TABLE test_replica_identity5;
DROP TABLE test_replica_identity_othertable;
+DROP TABLE test_replica_identity_t3;
--
2.39.2
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-08 05:58 Amul Sul <sulamul@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
2 siblings, 0 replies; 21+ messages in thread
From: Amul Sul @ 2024-03-08 05:58 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Dean Rasheed <dean.a.rasheed@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; vignesh C <vignesh21@gmail.com>
On Thu, Mar 7, 2024 at 11:02 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
> On 2024-Mar-07, Alvaro Herrera wrote:
>
> > Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> > RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> > logical replication would continue to not know about this PK, which
> > perhaps is what we want. I'll do some testing with this.
>
> This seems to work okay.
>
Thank you for working on this, the patch works nicely.
Regards,
Amul
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-08 11:18 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
2 siblings, 0 replies; 21+ messages in thread
From: Dean Rasheed @ 2024-03-08 11:18 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On Thu, 7 Mar 2024 at 17:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> This seems to work okay.
>
Yes, this looks good. I tested it against CREATE TABLE ... LIKE, and
it worked as expected. It might be worth adding a test case for that,
to ensure that it doesn't get broken in the future. Do we also want a
test case that does what pg_dump would do:
- Add a NOT NULL constraint
- Add a deferrable PK constraint
- Drop the NOT NULL constraint
- Check the column is still not nullable
Looking at rel.h, I think that the new field should probably come
after rd_pkindex, under the comment "data managed by
RelationGetIndexList", and have its own comment.
Also, if I'm nitpicking, the new field and local variables should use
the term "deferrable" rather than "deferred". A DEFERRABLE constraint
can be set to be either DEFERRED or IMMEDIATE within a transaction,
but "deferrable" is the right term to use to describe the persistent
property of an index/constraint that can be deferred. (The same
objection applies to the field name "indimmediate", but it's too late
to change that.)
Also, for neatness/consistency, the new field should probably be reset
in load_relcache_init_file(), alongside rd_pkindex, though I don't
think it can matter in practice.
Regards,
Dean
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-05-14 14:08 Robert Haas <robertmhaas@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
2 siblings, 1 reply; 21+ messages in thread
From: Robert Haas @ 2024-05-14 14:08 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Dean Rasheed <dean.a.rasheed@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On Thu, Mar 7, 2024 at 12:32 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2024-Mar-07, Alvaro Herrera wrote:
> > Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> > RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> > logical replication would continue to not know about this PK, which
> > perhaps is what we want. I'll do some testing with this.
>
> This seems to work okay.
There is a CommitFest entry for this patch. Should that entry be
closed in view of the not-NULL revert
(6f8bb7c1e9610dd7af20cdaf74c4ff6e6d678d44)?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-05-14 14:42 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2024-05-14 14:42 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Dean Rasheed <dean.a.rasheed@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On 2024-May-14, Robert Haas wrote:
> On Thu, Mar 7, 2024 at 12:32 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2024-Mar-07, Alvaro Herrera wrote:
> > > Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> > > RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> > > logical replication would continue to not know about this PK, which
> > > perhaps is what we want. I'll do some testing with this.
> >
> > This seems to work okay.
>
> There is a CommitFest entry for this patch. Should that entry be
> closed in view of the not-NULL revert
> (6f8bb7c1e9610dd7af20cdaf74c4ff6e6d678d44)?
Uhmm, I didn't realize there was a CF entry. I don't know why it was
there; this should have been an open item, not a bugfix CF entry.
This had already been committed as 270af6f0df76 (the day before it was
sent to the next commitfest). This commit wasn't included in the
reverted set, though, so you still get deferrable PKs from
RelationGetIndexList. I don't think this is necessarily a bad thing,
though these don't have any usefulness as things stand (and if we deal
with PKs by forcing not-null constraints to be underneath, then we won't
need them either).
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-05-14 14:55 Robert Haas <robertmhaas@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 21+ messages in thread
From: Robert Haas @ 2024-05-14 14:55 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Dean Rasheed <dean.a.rasheed@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On Tue, May 14, 2024 at 10:42 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> This had already been committed as 270af6f0df76 (the day before it was
> sent to the next commitfest). This commit wasn't included in the
> reverted set, though, so you still get deferrable PKs from
> RelationGetIndexList. I don't think this is necessarily a bad thing,
> though these don't have any usefulness as things stand (and if we deal
> with PKs by forcing not-null constraints to be underneath, then we won't
> need them either).
So, are you saying this should be marked Committed in the commitfest?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-05-14 15:11 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2024-05-14 15:11 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Dean Rasheed <dean.a.rasheed@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On 2024-May-14, Robert Haas wrote:
> On Tue, May 14, 2024 at 10:42 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > This had already been committed as 270af6f0df76 (the day before it was
> > sent to the next commitfest). This commit wasn't included in the
> > reverted set, though, so you still get deferrable PKs from
> > RelationGetIndexList. I don't think this is necessarily a bad thing,
> > though these don't have any usefulness as things stand (and if we deal
> > with PKs by forcing not-null constraints to be underneath, then we won't
> > need them either).
>
> So, are you saying this should be marked Committed in the commitfest?
Yeah. I've done so.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Pero la cosa no es muy grave ..." (le petit Nicolas -- René Goscinny)
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-05-14 15:17 Robert Haas <robertmhaas@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 21+ messages in thread
From: Robert Haas @ 2024-05-14 15:17 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Dean Rasheed <dean.a.rasheed@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; pgsql-hackers; Amul Sul <sulamul@gmail.com>; vignesh C <vignesh21@gmail.com>
On Tue, May 14, 2024 at 11:11 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > So, are you saying this should be marked Committed in the commitfest?
>
> Yeah. I've done so.
Great, thanks.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2024-05-14 15:17 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-09-20 12:58 Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Amul Sul <sulamul@gmail.com>
2023-09-20 14:59 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-09-22 10:55 ` Amul Sul <sulamul@gmail.com>
2024-01-20 02:24 ` vignesh C <vignesh21@gmail.com>
2024-01-23 12:11 ` Amul Sul <sulamul@gmail.com>
2024-03-04 12:34 ` Aleksander Alekseev <aleksander@timescale.com>
2024-03-04 13:50 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2024-03-05 12:36 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-03-05 14:58 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2024-03-05 17:18 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-03-07 13:00 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-03-07 13:42 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2024-03-07 15:10 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-03-07 17:32 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-03-08 05:58 ` Amul Sul <sulamul@gmail.com>
2024-03-08 11:18 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2024-05-14 14:08 ` Robert Haas <robertmhaas@gmail.com>
2024-05-14 14:42 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-05-14 14:55 ` Robert Haas <robertmhaas@gmail.com>
2024-05-14 15:11 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-05-14 15:17 ` Robert Haas <robertmhaas@gmail.com>
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