On Tuesday, November 19, 2024 3:06 AM Shlok Kyal <shlok.kyal....@gmail.com>
wrote:
>
> I have fixed the comments and attached an updated patch.
Thanks for the patch.
I slightly refactored the codes a bit:
* make the codes in replident_has_unpublished_gen_col()
consistent with other similar functions.
* Avoid unnecessary operations when there are no generated columns
In the table.
* Improve the loop by traversing the replica identity columns instead. I think
it looks clearer this way and better aligns with the purpose of the
replident_has_unpublished_gen_col function.
* Some cosmetic changes in the comments.
Please check the attached diff. Feel free to merge if it looks
acceptable to you.
Best Regards,
Hou zj
From 553dc53ec14b6ed8b898992f6b2085cbe12e5408 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.f...@cn.fujitsu.com>
Date: Tue, 19 Nov 2024 11:13:13 +0800
Subject: [PATCH v2] improve logic
---
src/backend/commands/publicationcmds.c | 29 ++++++++++++++------------
src/backend/utils/cache/relcache.c | 2 +-
2 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c
b/src/backend/commands/publicationcmds.c
index 8e6a61c997..053877c524 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -455,11 +455,16 @@ replident_has_unpublished_gen_col(Oid pubid, Relation
relation, List *ancestors,
bool
pubviaroot)
{
Oid relid = RelationGetRelid(relation);
- Oid publish_as_relid;
+ Oid publish_as_relid = RelationGetRelid(relation);
bool result = false;
bool found;
Publication *pub;
+ /* Return if the table does not contain any generated columns */
+ if (!relation->rd_att->constr ||
+ !relation->rd_att->constr->has_generated_stored)
+ return false;
+
/*
* For a partition, if pubviaroot is true, find the topmost ancestor
that
* is published via this publication as we need to use its column list
for
@@ -475,38 +480,36 @@ replident_has_unpublished_gen_col(Oid pubid, Relation
relation, List *ancestors,
if (!OidIsValid(publish_as_relid))
publish_as_relid = relid;
}
- else
- publish_as_relid = relid;
pub = GetPublication(pubid);
found = check_and_fetch_column_list(pub, publish_as_relid, NULL, NULL);
if (!found)
{
- Bitmapset *idattrs = NULL;
TupleDesc desc = RelationGetDescr(relation);
+ Bitmapset *idattrs;
+ int x;
/*
* REPLICA IDENTITY can be FULL only if there is no column list
for
* publication. If REPLICA IDENTITY is set as FULL and relation
has a
* generated column we should error out.
*/
- if (relation->rd_rel->relreplident == REPLICA_IDENTITY_FULL &&
- relation->rd_att->constr &&
- relation->rd_att->constr->has_generated_stored)
+ if (relation->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
return true;
+ /* Remember columns that are part of the REPLICA IDENTITY */
idattrs = RelationGetIndexAttrBitmap(relation,
INDEX_ATTR_BITMAP_IDENTITY_KEY);
- for (int i = 0; i < desc->natts; i++)
+ x = -1;
+ while ((x = bms_next_member(idattrs, x)) >= 0)
{
- Form_pg_attribute att = TupleDescAttr(desc, i);
+ AttrNumber attnum = (x +
FirstLowInvalidHeapAttributeNumber);
+ Form_pg_attribute att = TupleDescAttr(desc, attnum - 1);
- /* check if generated column is part of REPLICA
IDENTITY */
- if (!att->attisdropped && att->attgenerated &&
- bms_is_member(att->attnum -
FirstLowInvalidHeapAttributeNumber,
- idattrs))
+ /* Check if generated column is part of REPLICA
IDENTITY */
+ if (!att->attisdropped && att->attgenerated)
{
result = true;
break;
diff --git a/src/backend/utils/cache/relcache.c
b/src/backend/utils/cache/relcache.c
index 1e4cf99e85..be8f8eea8f 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5832,7 +5832,7 @@ RelationBuildPublicationDesc(Relation relation,
PublicationDesc *pubdesc)
}
/*
- * Check if all columns which are part of the REPLICA IDENTITY
is
+ * Check if all generated columns included in the REPLICA
IDENTITY are
* published.
*/
if (!pubform->pubgencols &&
--
2.30.0.windows.2
