public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Antonin Houska <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Subject: Re: repack: fix a bug to reject deferrable primary key fallback for concurrent mode
Date: Tue, 21 Apr 2026 10:38:05 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <65564.1776696735@localhost>
References: <[email protected]>
<65564.1776696735@localhost>
> On Apr 20, 2026, at 22:52, Antonin Houska <[email protected]> wrote:
>
> Chao Li <[email protected]> wrote:
>
>> I am continuing to test REPACK, and I found another issue.
>>
>> In check_concurrent_repack_requirements(), if a table has no replica identity index, the code falls back to using the primary key if one exists. The problem is that a deferrable primary key cannot be used for this purpose. WAL generation does not consider a deferrable primary key to be a replica identity, so concurrent mode may not receive enough old tuple information to replay concurrent changes.
>
> Thanks for finding it, this is certainly a problem.
>
> I'm just thinking if it's worth a separate error message.
> RelationGetIndexList() just ignores the deferrable PK
>
> if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex) && !pkdeferrable)
> relation->rd_replidindex = pkeyIndex;
>
> and if there's no other suitable index, the result is that there is no
> identity index for the table. So the change attached here should be consistent
> with this approach.
>
> --
> Antonin Houska
> Web: https://www.cybertec-postgresql.com
>
Hi Antonin,
Thanks for your review. I guess you read the v1 patch. In v2, I have switched to use GetRelationIdentityOrPK() that Zhijie suggested, which has covered RelationGetIndexList() and all checks, so that code is simplified, and there is no longer a separate error message.
As CFBot asked for, rebased to v3, nothing changed from v2. Can you please take a look at v3 again?
BTW, could you please also take a look at [1] that is a comment-only change for repack.
[1] https://postgr.es/m/[email protected]
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v3-0001-Reject-deferrable-primary-key-fallback-in-REPACK-.patch (7.1K, 2-v3-0001-Reject-deferrable-primary-key-fallback-in-REPACK-.patch)
download | inline diff:
From fdca43ea733a269a5fdb7621b26d16841eddb0c2 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Fri, 17 Apr 2026 11:12:36 +0800
Subject: [PATCH v3] Reject deferrable primary key fallback in REPACK
CONCURRENTLY
REPACK CONCURRENTLY uses logical decoding to collect concurrent
changes and then replays them on the new heap. To locate rows for
UPDATE and DELETE replay, it requires an identity index.
When RelationGetReplicaIndex() returned InvalidOid, the code fell
back to rel->rd_pkindex if a primary key existed. That is not safe for
deferrable primary keys. Such indexes are not considered replica
identity indexes by WAL generation, so logical decoding may not provide
the old tuple needed by the repack output plugin.
This can make replay fail later with errors such as "incomplete delete
info" from the decoding worker.
This change switches to use GetRelationIdentityOrPK() that excludes
deferrable primary key.
Author: Chao Li <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Reviewed-by: Antonin Houska <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/repack.c | 15 +++++----
src/test/regress/expected/cluster.out | 44 ++++++++++++++++++++++++++-
src/test/regress/sql/cluster.sql | 39 +++++++++++++++++++++++-
3 files changed, 88 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 67364cc60e3..76d84180acc 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -62,6 +62,7 @@
#include "miscadmin.h"
#include "optimizer/optimizer.h"
#include "pgstat.h"
+#include "replication/logicalrelation.h"
#include "storage/bufmgr.h"
#include "storage/lmgr.h"
#include "storage/predicate.h"
@@ -919,14 +920,12 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
/*
* Obtain the replica identity index -- either one that has been set
- * explicitly, or the primary key. If none of these cases apply, the
- * table cannot be repacked concurrently. It might be possible to have
- * repack work with a FULL replica identity; however that requires more
- * work and is not implemented yet.
- */
- ident_idx = RelationGetReplicaIndex(rel);
- if (!OidIsValid(ident_idx) && OidIsValid(rel->rd_pkindex))
- ident_idx = rel->rd_pkindex;
+ * explicitly, or a non-deferrable primary key. If none of these cases
+ * apply, the table cannot be repacked concurrently. It might be possible
+ * to have repack work with a FULL replica identity; however that requires
+ * more work and is not implemented yet.
+ */
+ ident_idx = GetRelationIdentityOrPK(rel);
if (!OidIsValid(ident_idx))
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 71270134985..e7cb89c5298 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -822,10 +822,52 @@ ORDER BY o.relname;
clstr_3
(2 rows)
--- concurrently
+--
+-- Check concurrent mode requirements
+--
+-- Doesn't support catalog tables
REPACK (CONCURRENTLY) pg_class;
ERROR: cannot repack relation "pg_class"
HINT: REPACK CONCURRENTLY is not supported for catalog relations.
+-- Only support permanent tables, temp and unlogged tables are not supported
+CREATE TEMP TABLE repack_conc_temp (i int PRIMARY KEY);
+REPACK (CONCURRENTLY) repack_conc_temp;
+ERROR: cannot repack relation "repack_conc_temp"
+HINT: REPACK CONCURRENTLY is only allowed for permanent relations.
+DROP TABLE repack_conc_temp;
+CREATE UNLOGGED TABLE repack_conc_unlogged (i int PRIMARY KEY);
+REPACK (CONCURRENTLY) repack_conc_unlogged;
+ERROR: cannot repack relation "repack_conc_unlogged"
+HINT: REPACK CONCURRENTLY is only allowed for permanent relations.
+DROP TABLE repack_conc_unlogged;
+-- Doesn't support tables without a primary key or replica identity index
+CREATE TABLE repack_conc_noident (i int);
+REPACK (CONCURRENTLY) repack_conc_noident;
+ERROR: cannot process relation "repack_conc_noident"
+HINT: Relation "repack_conc_noident" has no identity index.
+DROP TABLE repack_conc_noident;
+-- Doesn't support TOAST tables directly
+CREATE TABLE repack_conc_toast (t text);
+SELECT reltoastrelid::regclass AS toast_rel
+FROM pg_class WHERE oid = 'repack_conc_toast'::regclass \gset
+\set VERBOSITY sqlstate
+REPACK (CONCURRENTLY) :toast_rel;
+ERROR: 0A000
+\set VERBOSITY default
+DROP TABLE repack_conc_toast;
+-- Doesn't support tables with REPLICA IDENTITY NOTHING, even if they have a primary key
+CREATE TABLE repack_conc_nothing (i int PRIMARY KEY);
+ALTER TABLE repack_conc_nothing REPLICA IDENTITY NOTHING;
+REPACK (CONCURRENTLY) repack_conc_nothing;
+ERROR: cannot repack relation "repack_conc_nothing"
+HINT: Relation "repack_conc_nothing" has insufficient replication identity.
+DROP TABLE repack_conc_nothing;
+-- Doesn't support tables with deferrable primary keys
+CREATE TABLE repack_conc_deferrable (i int PRIMARY KEY DEFERRABLE);
+REPACK (CONCURRENTLY) repack_conc_deferrable;
+ERROR: cannot process relation "repack_conc_deferrable"
+HINT: Relation "repack_conc_deferrable" has no identity index.
+DROP TABLE repack_conc_deferrable;
-- clean up
DROP TABLE clustertest;
DROP TABLE clstr_1;
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 6746236ffec..e5e369050bb 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -393,9 +393,46 @@ JOIN relnodes_new n ON o.relname = n.relname
WHERE o.relfilenode <> n.relfilenode
ORDER BY o.relname;
--- concurrently
+--
+-- Check concurrent mode requirements
+--
+
+-- Doesn't support catalog tables
REPACK (CONCURRENTLY) pg_class;
+-- Only support permanent tables, temp and unlogged tables are not supported
+CREATE TEMP TABLE repack_conc_temp (i int PRIMARY KEY);
+REPACK (CONCURRENTLY) repack_conc_temp;
+DROP TABLE repack_conc_temp;
+CREATE UNLOGGED TABLE repack_conc_unlogged (i int PRIMARY KEY);
+REPACK (CONCURRENTLY) repack_conc_unlogged;
+DROP TABLE repack_conc_unlogged;
+
+-- Doesn't support tables without a primary key or replica identity index
+CREATE TABLE repack_conc_noident (i int);
+REPACK (CONCURRENTLY) repack_conc_noident;
+DROP TABLE repack_conc_noident;
+
+-- Doesn't support TOAST tables directly
+CREATE TABLE repack_conc_toast (t text);
+SELECT reltoastrelid::regclass AS toast_rel
+FROM pg_class WHERE oid = 'repack_conc_toast'::regclass \gset
+\set VERBOSITY sqlstate
+REPACK (CONCURRENTLY) :toast_rel;
+\set VERBOSITY default
+DROP TABLE repack_conc_toast;
+
+-- Doesn't support tables with REPLICA IDENTITY NOTHING, even if they have a primary key
+CREATE TABLE repack_conc_nothing (i int PRIMARY KEY);
+ALTER TABLE repack_conc_nothing REPLICA IDENTITY NOTHING;
+REPACK (CONCURRENTLY) repack_conc_nothing;
+DROP TABLE repack_conc_nothing;
+
+-- Doesn't support tables with deferrable primary keys
+CREATE TABLE repack_conc_deferrable (i int PRIMARY KEY DEFERRABLE);
+REPACK (CONCURRENTLY) repack_conc_deferrable;
+DROP TABLE repack_conc_deferrable;
+
-- clean up
DROP TABLE clustertest;
DROP TABLE clstr_1;
--
2.50.1 (Apple Git-155)
view thread (10+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: repack: fix a bug to reject deferrable primary key fallback for concurrent mode
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox