public inbox for [email protected]help / color / mirror / Atom feed
PublicationActions - use bit flags. 12+ messages / 6 participants [nested] [flat]
* PublicationActions - use bit flags. @ 2021-12-20 00:18 Peter Smith <[email protected]> 0 siblings, 3 replies; 12+ messages in thread From: Peter Smith @ 2021-12-20 00:18 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> For some reason the current HEAD PublicationActions is a struct of boolean representing combinations of the 4 different "publication actions". I felt it is more natural to implement boolean flag combinations using a bitmask instead of a struct of bools. IMO using the bitmask also simplifies assignment and checking of said flags. PSA a small patch for this. Thoughts? ------ Kind Regards, Peter Smith. Fujitsu Australia Attachments: [application/octet-stream] v1-0001-PublicationActions-use-bit-flags.patch (13.4K, ../../CAHut+Pt2hjRJxfuF9_GUcBVg=nhk3G+WNsNkPtP2d+4CF4e4Pg@mail.gmail.com/2-v1-0001-PublicationActions-use-bit-flags.patch) download | inline diff: From 69769779608d462b162d04c912a59c1601bf963e Mon Sep 17 00:00:00 2001 From: Peter Smith <[email protected]> Date: Mon, 20 Dec 2021 09:28:08 +1100 Subject: [PATCH v1] PublicationActions - use bit flags. The current PublicationActions is a struct of boolean representing combinations of the 4 different "publication actions". IMO it is more natural to implement these flag combinations using a bitmask instead of a struct of bools. This patch keeps the PublicationActions typedef but changes it to be unsigned int, so the previous structure members are all replaced now by bit operations. --- src/backend/catalog/pg_publication.c | 9 ++++--- src/backend/commands/publicationcmds.c | 38 ++++++++++++++--------------- src/backend/executor/execReplication.c | 6 ++--- src/backend/replication/pgoutput/pgoutput.c | 24 ++++++------------ src/backend/utils/cache/relcache.c | 20 +++++++-------- src/include/catalog/pg_publication.h | 13 +++++----- src/include/utils/relcache.h | 4 +-- 7 files changed, 51 insertions(+), 63 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 62f10bc..dbc5d7a 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -783,10 +783,11 @@ GetPublication(Oid pubid) pub->oid = pubid; pub->name = pstrdup(NameStr(pubform->pubname)); pub->alltables = pubform->puballtables; - pub->pubactions.pubinsert = pubform->pubinsert; - pub->pubactions.pubupdate = pubform->pubupdate; - pub->pubactions.pubdelete = pubform->pubdelete; - pub->pubactions.pubtruncate = pubform->pubtruncate; + pub->pubactions = 0; + if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT; + if (pubform->pubupdate) pub->pubactions |= PUBACTION_UPDATE; + if (pubform->pubdelete) pub->pubactions |= PUBACTION_DELETE; + if (pubform->pubtruncate) pub->pubactions |= PUBACTION_TRUNCATE; pub->pubviaroot = pubform->pubviaroot; ReleaseSysCache(tup); diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 404bb5d..d509e4b 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -73,10 +73,7 @@ parse_publication_options(ParseState *pstate, *publish_via_partition_root_given = false; /* defaults */ - pubactions->pubinsert = true; - pubactions->pubupdate = true; - pubactions->pubdelete = true; - pubactions->pubtruncate = true; + *pubactions = PUBACTION_ALL; *publish_via_partition_root = false; /* Parse options */ @@ -97,10 +94,7 @@ parse_publication_options(ParseState *pstate, * If publish option was given only the explicitly listed actions * should be published. */ - pubactions->pubinsert = false; - pubactions->pubupdate = false; - pubactions->pubdelete = false; - pubactions->pubtruncate = false; + *pubactions = 0; *publish_given = true; publish = defGetString(defel); @@ -116,13 +110,13 @@ parse_publication_options(ParseState *pstate, char *publish_opt = (char *) lfirst(lc); if (strcmp(publish_opt, "insert") == 0) - pubactions->pubinsert = true; + *pubactions |= PUBACTION_INSERT; else if (strcmp(publish_opt, "update") == 0) - pubactions->pubupdate = true; + *pubactions |= PUBACTION_UPDATE; else if (strcmp(publish_opt, "delete") == 0) - pubactions->pubdelete = true; + *pubactions |= PUBACTION_DELETE; else if (strcmp(publish_opt, "truncate") == 0) - pubactions->pubtruncate = true; + *pubactions |= PUBACTION_TRUNCATE; else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -299,13 +293,13 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(stmt->for_all_tables); values[Anum_pg_publication_pubinsert - 1] = - BoolGetDatum(pubactions.pubinsert); + BoolGetDatum(pubactions & PUBACTION_INSERT); values[Anum_pg_publication_pubupdate - 1] = - BoolGetDatum(pubactions.pubupdate); + BoolGetDatum(pubactions & PUBACTION_UPDATE); values[Anum_pg_publication_pubdelete - 1] = - BoolGetDatum(pubactions.pubdelete); + BoolGetDatum(pubactions & PUBACTION_DELETE); values[Anum_pg_publication_pubtruncate - 1] = - BoolGetDatum(pubactions.pubtruncate); + BoolGetDatum(pubactions & PUBACTION_TRUNCATE); values[Anum_pg_publication_pubviaroot - 1] = BoolGetDatum(publish_via_partition_root); @@ -406,16 +400,20 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, if (publish_given) { - values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactions.pubinsert); + values[Anum_pg_publication_pubinsert - 1] = + BoolGetDatum(pubactions & PUBACTION_INSERT); replaces[Anum_pg_publication_pubinsert - 1] = true; - values[Anum_pg_publication_pubupdate - 1] = BoolGetDatum(pubactions.pubupdate); + values[Anum_pg_publication_pubupdate - 1] = + BoolGetDatum(pubactions & PUBACTION_UPDATE); replaces[Anum_pg_publication_pubupdate - 1] = true; - values[Anum_pg_publication_pubdelete - 1] = BoolGetDatum(pubactions.pubdelete); + values[Anum_pg_publication_pubdelete - 1] = + BoolGetDatum(pubactions & PUBACTION_DELETE); replaces[Anum_pg_publication_pubdelete - 1] = true; - values[Anum_pg_publication_pubtruncate - 1] = BoolGetDatum(pubactions.pubtruncate); + values[Anum_pg_publication_pubtruncate - 1] = + BoolGetDatum(pubactions & PUBACTION_TRUNCATE); replaces[Anum_pg_publication_pubtruncate - 1] = true; } diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 574d7d2..4e2f9d5 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -567,7 +567,7 @@ ExecSimpleRelationDelete(ResultRelInfo *resultRelInfo, void CheckCmdReplicaIdentity(Relation rel, CmdType cmd) { - PublicationActions *pubactions; + PublicationActions pubactions; /* We only need to do checks for UPDATE and DELETE. */ if (cmd != CMD_UPDATE && cmd != CMD_DELETE) @@ -584,13 +584,13 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd) * Check if the table publishes UPDATES or DELETES. */ pubactions = GetRelationPublicationActions(rel); - if (cmd == CMD_UPDATE && pubactions->pubupdate) + if (cmd == CMD_UPDATE && (pubactions & PUBACTION_UPDATE)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot update table \"%s\" because it does not have a replica identity and publishes updates", RelationGetRelationName(rel)), errhint("To enable updating the table, set REPLICA IDENTITY using ALTER TABLE."))); - else if (cmd == CMD_DELETE && pubactions->pubdelete) + else if (cmd == CMD_DELETE && (pubactions & PUBACTION_DELETE)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot delete from table \"%s\" because it does not have a replica identity and publishes deletes", diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 6f6a203..11a0277 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -653,15 +653,15 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, switch (change->action) { case REORDER_BUFFER_CHANGE_INSERT: - if (!relentry->pubactions.pubinsert) + if (!(relentry->pubactions & PUBACTION_INSERT)) return; break; case REORDER_BUFFER_CHANGE_UPDATE: - if (!relentry->pubactions.pubupdate) + if (!(relentry->pubactions & PUBACTION_UPDATE)) return; break; case REORDER_BUFFER_CHANGE_DELETE: - if (!relentry->pubactions.pubdelete) + if (!(relentry->pubactions & PUBACTION_DELETE)) return; break; default: @@ -796,7 +796,7 @@ pgoutput_truncate(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, relentry = get_rel_sync_entry(data, relid); - if (!relentry->pubactions.pubtruncate) + if (!(relentry->pubactions & PUBACTION_TRUNCATE)) continue; /* @@ -1139,8 +1139,7 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) entry->schema_sent = false; entry->streamed_txns = NIL; entry->replicate_valid = false; - entry->pubactions.pubinsert = entry->pubactions.pubupdate = - entry->pubactions.pubdelete = entry->pubactions.pubtruncate = false; + entry->pubactions = 0; entry->publish_as_relid = InvalidOid; entry->map = NULL; /* will be set by maybe_send_schema() if * needed */ @@ -1239,14 +1238,10 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) if (publish && (relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)) { - entry->pubactions.pubinsert |= pub->pubactions.pubinsert; - entry->pubactions.pubupdate |= pub->pubactions.pubupdate; - entry->pubactions.pubdelete |= pub->pubactions.pubdelete; - entry->pubactions.pubtruncate |= pub->pubactions.pubtruncate; + entry->pubactions |= pub->pubactions; } - if (entry->pubactions.pubinsert && entry->pubactions.pubupdate && - entry->pubactions.pubdelete && entry->pubactions.pubtruncate) + if (entry->pubactions == PUBACTION_ALL) break; } @@ -1387,10 +1382,7 @@ rel_sync_cache_publication_cb(Datum arg, int cacheid, uint32 hashvalue) * There might be some relations dropped from the publication so we * don't need to publish the changes for them. */ - entry->pubactions.pubinsert = false; - entry->pubactions.pubupdate = false; - entry->pubactions.pubdelete = false; - entry->pubactions.pubtruncate = false; + entry->pubactions = 0; } } diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 105d8d4..c6cb2e5 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -5524,14 +5524,14 @@ RelationGetExclusionInfo(Relation indexRelation, /* * Get publication actions for the given relation. */ -struct PublicationActions * +PublicationActions GetRelationPublicationActions(Relation relation) { List *puboids; ListCell *lc; MemoryContext oldcxt; Oid schemaid; - PublicationActions *pubactions = palloc0(sizeof(PublicationActions)); + PublicationActions pubactions = 0; /* * If not publishable, it publishes no actions. (pgoutput_change() will @@ -5541,8 +5541,7 @@ GetRelationPublicationActions(Relation relation) return pubactions; if (relation->rd_pubactions) - return memcpy(pubactions, relation->rd_pubactions, - sizeof(PublicationActions)); + return *relation->rd_pubactions; /* Fetch the publication membership info. */ puboids = GetRelationPublications(RelationGetRelid(relation)); @@ -5581,10 +5580,10 @@ GetRelationPublicationActions(Relation relation) pubform = (Form_pg_publication) GETSTRUCT(tup); - pubactions->pubinsert |= pubform->pubinsert; - pubactions->pubupdate |= pubform->pubupdate; - pubactions->pubdelete |= pubform->pubdelete; - pubactions->pubtruncate |= pubform->pubtruncate; + if (pubform->pubinsert) pubactions |= PUBACTION_INSERT; + if (pubform->pubupdate) pubactions |= PUBACTION_UPDATE; + if (pubform->pubdelete) pubactions |= PUBACTION_DELETE; + if (pubform->pubtruncate) pubactions |= PUBACTION_TRUNCATE; ReleaseSysCache(tup); @@ -5592,8 +5591,7 @@ GetRelationPublicationActions(Relation relation) * If we know everything is replicated, there is no point to check for * other publications. */ - if (pubactions->pubinsert && pubactions->pubupdate && - pubactions->pubdelete && pubactions->pubtruncate) + if (pubactions == PUBACTION_ALL) break; } @@ -5606,7 +5604,7 @@ GetRelationPublicationActions(Relation relation) /* Now save copy of the actions in the relcache entry. */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); relation->rd_pubactions = palloc(sizeof(PublicationActions)); - memcpy(relation->rd_pubactions, pubactions, sizeof(PublicationActions)); + *relation->rd_pubactions = pubactions; MemoryContextSwitchTo(oldcxt); return pubactions; diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 902f2f2..376e49c 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -66,13 +66,12 @@ typedef FormData_pg_publication *Form_pg_publication; DECLARE_UNIQUE_INDEX_PKEY(pg_publication_oid_index, 6110, PublicationObjectIndexId, on pg_publication using btree(oid oid_ops)); DECLARE_UNIQUE_INDEX(pg_publication_pubname_index, 6111, PublicationNameIndexId, on pg_publication using btree(pubname name_ops)); -typedef struct PublicationActions -{ - bool pubinsert; - bool pubupdate; - bool pubdelete; - bool pubtruncate; -} PublicationActions; +#define PUBACTION_INSERT (1 << 0) +#define PUBACTION_UPDATE (1 << 1) +#define PUBACTION_DELETE (1 << 2) +#define PUBACTION_TRUNCATE (1 << 3) +#define PUBACTION_ALL (PUBACTION_INSERT | PUBACTION_UPDATE | PUBACTION_DELETE | PUBACTION_TRUNCATE) +typedef unsigned PublicationActions; typedef struct Publication { diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index 82316bb..9bd8b9b 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -74,8 +74,8 @@ extern void RelationGetExclusionInfo(Relation indexRelation, extern void RelationInitIndexAccessInfo(Relation relation); /* caller must include pg_publication.h */ -struct PublicationActions; -extern struct PublicationActions *GetRelationPublicationActions(Relation relation); +typedef unsigned PublicationActions; +extern PublicationActions GetRelationPublicationActions(Relation relation); extern void RelationInitTableAccessMethod(Relation relation); -- 1.8.3.1 ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-20 05:10 Greg Nancarrow <[email protected]> parent: Peter Smith <[email protected]> 2 siblings, 0 replies; 12+ messages in thread From: Greg Nancarrow @ 2021-12-20 05:10 UTC (permalink / raw) To: Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, Dec 20, 2021 at 11:19 AM Peter Smith <[email protected]> wrote: > > For some reason the current HEAD PublicationActions is a struct of > boolean representing combinations of the 4 different "publication > actions". > > I felt it is more natural to implement boolean flag combinations using > a bitmask instead of a struct of bools. IMO using the bitmask also > simplifies assignment and checking of said flags. > > PSA a small patch for this. > > Thoughts? > +1 I think the bit flags are a more natural fit, and also the patch removes the unnecessary use of a palloc'd tiny struct to return the PublicationActions (which also currently isn't explicitly pfree'd). Regards, Greg Nancarrow Fujitsu Australia ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-20 13:58 Peter Eisentraut <[email protected]> parent: Peter Smith <[email protected]> 2 siblings, 2 replies; 12+ messages in thread From: Peter Eisentraut @ 2021-12-20 13:58 UTC (permalink / raw) To: Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]> On 20.12.21 01:18, Peter Smith wrote: > For some reason the current HEAD PublicationActions is a struct of > boolean representing combinations of the 4 different "publication > actions". > > I felt it is more natural to implement boolean flag combinations using > a bitmask instead of a struct of bools. IMO using the bitmask also > simplifies assignment and checking of said flags. I don't see why this is better. It just makes the code longer and adds more punctuation and reduces type safety. ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-20 16:56 Tom Lane <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Tom Lane @ 2021-12-20 16:56 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]> Peter Eisentraut <[email protected]> writes: > On 20.12.21 01:18, Peter Smith wrote: >> I felt it is more natural to implement boolean flag combinations using >> a bitmask instead of a struct of bools. IMO using the bitmask also >> simplifies assignment and checking of said flags. > I don't see why this is better. It just makes the code longer and adds > more punctuation and reduces type safety. It makes the code shorter in places where you need to process all the flags at once, but I agree it's not really an improvement elsewhere. Not sure if it's worth changing. One thing I noted is that the duplicate PublicationActions typedefs will certainly draw warnings, if not hard errors, from some compilers. You could get around that by removing the typedefs altogether and just using "int", which'd be more consistent with our usual practices anyway. But it does play into Peter's objection about type safety. regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-20 17:14 Alvaro Herrera <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 1 reply; 12+ messages in thread From: Alvaro Herrera @ 2021-12-20 17:14 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]> On 2021-Dec-20, Peter Eisentraut wrote: > I don't see why this is better. It just makes the code longer and adds more > punctuation and reduces type safety. Removing one palloc is I think the most important consequence ... probably not a big deal though. I think we could change the memcpy calls to struct assignment, as that would look a bit cleaner, and call it a day. One thing I would not like would be to change the catalog representation from bools into an integer. We do that for pg_trigger.tgflags (IIRC) and it is horrible. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-21 00:19 Greg Nancarrow <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Greg Nancarrow @ 2021-12-21 00:19 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Dec 21, 2021 at 4:14 AM Alvaro Herrera <[email protected]> wrote: > > On 2021-Dec-20, Peter Eisentraut wrote: > > > I don't see why this is better. It just makes the code longer and adds more > > punctuation and reduces type safety. > > Removing one palloc is I think the most important consequence ... > probably not a big deal though. > > I think we could change the memcpy calls to struct assignment, as that > would look a bit cleaner, and call it a day. > I think we can all agree that returning PublicationActions as a palloc'd struct is unnecessary. I've attached a patch which addresses that and replaces a couple of memcpy()s with struct assignment, as suggested. Regards, Greg Nancarrow Fujitsu Australia Attachments: [application/octet-stream] get_rel_pubactions_improvement.patch (4.0K, ../../CAJcOf-d0T+9jy3ois4eytkX_NPpfWMiSA5a=23_5TXSiU26BEg@mail.gmail.com/2-get_rel_pubactions_improvement.patch) download | inline diff: diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 574d7d27fd..305f0c5594 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -567,7 +567,7 @@ ExecSimpleRelationDelete(ResultRelInfo *resultRelInfo, void CheckCmdReplicaIdentity(Relation rel, CmdType cmd) { - PublicationActions *pubactions; + PublicationActions pubactions; /* We only need to do checks for UPDATE and DELETE. */ if (cmd != CMD_UPDATE && cmd != CMD_DELETE) @@ -583,14 +583,14 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd) * * Check if the table publishes UPDATES or DELETES. */ - pubactions = GetRelationPublicationActions(rel); - if (cmd == CMD_UPDATE && pubactions->pubupdate) + GetRelationPublicationActions(rel, &pubactions); + if (cmd == CMD_UPDATE && pubactions.pubupdate) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot update table \"%s\" because it does not have a replica identity and publishes updates", RelationGetRelationName(rel)), errhint("To enable updating the table, set REPLICA IDENTITY using ALTER TABLE."))); - else if (cmd == CMD_DELETE && pubactions->pubdelete) + else if (cmd == CMD_DELETE && pubactions.pubdelete) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot delete from table \"%s\" because it does not have a replica identity and publishes deletes", diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 105d8d4601..e8efae6198 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -5524,25 +5524,29 @@ RelationGetExclusionInfo(Relation indexRelation, /* * Get publication actions for the given relation. */ -struct PublicationActions * -GetRelationPublicationActions(Relation relation) +void +GetRelationPublicationActions(Relation relation, PublicationActions *pubactions) { List *puboids; ListCell *lc; MemoryContext oldcxt; Oid schemaid; - PublicationActions *pubactions = palloc0(sizeof(PublicationActions)); /* * If not publishable, it publishes no actions. (pgoutput_change() will * ignore it.) */ if (!is_publishable_relation(relation)) - return pubactions; + { + memset(pubactions, 0, sizeof(*pubactions)); + return; + } if (relation->rd_pubactions) - return memcpy(pubactions, relation->rd_pubactions, - sizeof(PublicationActions)); + { + *pubactions = *relation->rd_pubactions; + return; + } /* Fetch the publication membership info. */ puboids = GetRelationPublications(RelationGetRelid(relation)); @@ -5568,6 +5572,7 @@ GetRelationPublicationActions(Relation relation) } puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + memset(pubactions, 0, sizeof(*pubactions)); foreach(lc, puboids) { Oid pubid = lfirst_oid(lc); @@ -5598,18 +5603,13 @@ GetRelationPublicationActions(Relation relation) } if (relation->rd_pubactions) - { pfree(relation->rd_pubactions); - relation->rd_pubactions = NULL; - } /* Now save copy of the actions in the relcache entry. */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); relation->rd_pubactions = palloc(sizeof(PublicationActions)); - memcpy(relation->rd_pubactions, pubactions, sizeof(PublicationActions)); + *relation->rd_pubactions = *pubactions; MemoryContextSwitchTo(oldcxt); - - return pubactions; } /* diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index 82316bba54..376638f484 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -75,7 +75,8 @@ extern void RelationInitIndexAccessInfo(Relation relation); /* caller must include pg_publication.h */ struct PublicationActions; -extern struct PublicationActions *GetRelationPublicationActions(Relation relation); +extern void GetRelationPublicationActions(Relation relation, + struct PublicationActions *pubactions); extern void RelationInitTableAccessMethod(Relation relation); ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-21 00:56 Tom Lane <[email protected]> parent: Greg Nancarrow <[email protected]> 0 siblings, 2 replies; 12+ messages in thread From: Tom Lane @ 2021-12-21 00:56 UTC (permalink / raw) To: Greg Nancarrow <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Peter Eisentraut <[email protected]>; Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]> Greg Nancarrow <[email protected]> writes: > I've attached a patch which addresses that and replaces a couple of > memcpy()s with struct assignment, as suggested. Removing this is not good: if (relation->rd_pubactions) - { pfree(relation->rd_pubactions); - relation->rd_pubactions = NULL; - } If the subsequent palloc fails, you've created a problem where there was none before. I do wonder why we have to palloc a constant-size substructure in the first place, especially one that is likely smaller than the pointer that points to it. Maybe the struct definition should be moved so that we can just declare it in-line in the relcache entry? regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-21 01:55 Greg Nancarrow <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Greg Nancarrow @ 2021-12-21 01:55 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Peter Eisentraut <[email protected]>; Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Dec 21, 2021 at 11:56 AM Tom Lane <[email protected]> wrote: > > Removing this is not good: > > if (relation->rd_pubactions) > - { > pfree(relation->rd_pubactions); > - relation->rd_pubactions = NULL; > - } > > If the subsequent palloc fails, you've created a problem where > there was none before. > Oops, yeah, I got carried away; if palloc() failed and called exit(), then it would end up crashing when trying to use/pfree rd_pubactions again. Better leave that line in ... > I do wonder why we have to palloc a constant-size substructure in > the first place, especially one that is likely smaller than the > pointer that points to it. Maybe the struct definition should be > moved so that we can just declare it in-line in the relcache entry? > I think currently it's effectively using the rd_pubactions pointer as a boolean flag to indicate whether the publication membership info has been fetched (so the bool flags are valid). I guess you'd need another bool flag if you wanted to make that struct in-line in the relcache entry. Regards, Greg Nancarrow Fujitsu Australia ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-29 04:21 Peter Smith <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Peter Smith @ 2021-12-29 04:21 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Greg Nancarrow <[email protected]>; Alvaro Herrera <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Dec 21, 2021 at 11:56 AM Tom Lane <[email protected]> wrote: > > Greg Nancarrow <[email protected]> writes: > > I've attached a patch which addresses that and replaces a couple of > > memcpy()s with struct assignment, as suggested. > > Removing this is not good: > > if (relation->rd_pubactions) > - { > pfree(relation->rd_pubactions); > - relation->rd_pubactions = NULL; > - } > > If the subsequent palloc fails, you've created a problem where > there was none before. > > I do wonder why we have to palloc a constant-size substructure in > the first place, especially one that is likely smaller than the > pointer that points to it. Maybe the struct definition should be > moved so that we can just declare it in-line in the relcache entry? > At the risk of flogging a dead horse, here is v2 of my original bit-flag replacement for the PublicationActions struct. This version introduces one more bit flag for the relcache status, and by doing so means all that code for Relation cache PublicationActions pointers and pallocs and context switches can just disappear... ------ Kind Regards, Peter Smith. Fujitsu Australia Attachments: [application/octet-stream] v2-0001-PublicationActions-use-bit-flags.patch (15.0K, ../../CAHut+Ptv-gA7=zvc0xmS+7iejRrk0aRj+Rar69Zh1Qm1KqvsGA@mail.gmail.com/2-v2-0001-PublicationActions-use-bit-flags.patch) download | inline diff: From fbe57036acd717f5fa3f8d2ee4de09fa555100ff Mon Sep 17 00:00:00 2001 From: Peter Smith <[email protected]> Date: Wed, 29 Dec 2021 14:54:42 +1100 Subject: [PATCH v2] PublicationActions - use bit flags. The current PublicationActions is a struct of boolean representing combinations of the 4 different "publication actions". IMO it is more natural to implement these flag combinations using a bitmask instead of a struct of bools. This patch keeps the PublicationActions typedef but changes it to be unsigned int, so the previous structure members are all replaced now by bit operations. The Relation cache is also replaced by a simple PublicationAction bitmask instead of a pointer to an allocated struct of pubaction flags. --- src/backend/catalog/pg_publication.c | 9 ++++--- src/backend/commands/publicationcmds.c | 38 +++++++++++++--------------- src/backend/executor/execReplication.c | 6 ++--- src/backend/replication/pgoutput/pgoutput.c | 24 ++++++------------ src/backend/utils/cache/relcache.c | 39 ++++++++++------------------- src/include/catalog/pg_publication.h | 14 +++++------ src/include/utils/rel.h | 2 +- src/include/utils/relcache.h | 3 +-- 8 files changed, 56 insertions(+), 79 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 62f10bc..dbc5d7a 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -783,10 +783,11 @@ GetPublication(Oid pubid) pub->oid = pubid; pub->name = pstrdup(NameStr(pubform->pubname)); pub->alltables = pubform->puballtables; - pub->pubactions.pubinsert = pubform->pubinsert; - pub->pubactions.pubupdate = pubform->pubupdate; - pub->pubactions.pubdelete = pubform->pubdelete; - pub->pubactions.pubtruncate = pubform->pubtruncate; + pub->pubactions = 0; + if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT; + if (pubform->pubupdate) pub->pubactions |= PUBACTION_UPDATE; + if (pubform->pubdelete) pub->pubactions |= PUBACTION_DELETE; + if (pubform->pubtruncate) pub->pubactions |= PUBACTION_TRUNCATE; pub->pubviaroot = pubform->pubviaroot; ReleaseSysCache(tup); diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 404bb5d..d509e4b 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -73,10 +73,7 @@ parse_publication_options(ParseState *pstate, *publish_via_partition_root_given = false; /* defaults */ - pubactions->pubinsert = true; - pubactions->pubupdate = true; - pubactions->pubdelete = true; - pubactions->pubtruncate = true; + *pubactions = PUBACTION_ALL; *publish_via_partition_root = false; /* Parse options */ @@ -97,10 +94,7 @@ parse_publication_options(ParseState *pstate, * If publish option was given only the explicitly listed actions * should be published. */ - pubactions->pubinsert = false; - pubactions->pubupdate = false; - pubactions->pubdelete = false; - pubactions->pubtruncate = false; + *pubactions = 0; *publish_given = true; publish = defGetString(defel); @@ -116,13 +110,13 @@ parse_publication_options(ParseState *pstate, char *publish_opt = (char *) lfirst(lc); if (strcmp(publish_opt, "insert") == 0) - pubactions->pubinsert = true; + *pubactions |= PUBACTION_INSERT; else if (strcmp(publish_opt, "update") == 0) - pubactions->pubupdate = true; + *pubactions |= PUBACTION_UPDATE; else if (strcmp(publish_opt, "delete") == 0) - pubactions->pubdelete = true; + *pubactions |= PUBACTION_DELETE; else if (strcmp(publish_opt, "truncate") == 0) - pubactions->pubtruncate = true; + *pubactions |= PUBACTION_TRUNCATE; else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -299,13 +293,13 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(stmt->for_all_tables); values[Anum_pg_publication_pubinsert - 1] = - BoolGetDatum(pubactions.pubinsert); + BoolGetDatum(pubactions & PUBACTION_INSERT); values[Anum_pg_publication_pubupdate - 1] = - BoolGetDatum(pubactions.pubupdate); + BoolGetDatum(pubactions & PUBACTION_UPDATE); values[Anum_pg_publication_pubdelete - 1] = - BoolGetDatum(pubactions.pubdelete); + BoolGetDatum(pubactions & PUBACTION_DELETE); values[Anum_pg_publication_pubtruncate - 1] = - BoolGetDatum(pubactions.pubtruncate); + BoolGetDatum(pubactions & PUBACTION_TRUNCATE); values[Anum_pg_publication_pubviaroot - 1] = BoolGetDatum(publish_via_partition_root); @@ -406,16 +400,20 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, if (publish_given) { - values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactions.pubinsert); + values[Anum_pg_publication_pubinsert - 1] = + BoolGetDatum(pubactions & PUBACTION_INSERT); replaces[Anum_pg_publication_pubinsert - 1] = true; - values[Anum_pg_publication_pubupdate - 1] = BoolGetDatum(pubactions.pubupdate); + values[Anum_pg_publication_pubupdate - 1] = + BoolGetDatum(pubactions & PUBACTION_UPDATE); replaces[Anum_pg_publication_pubupdate - 1] = true; - values[Anum_pg_publication_pubdelete - 1] = BoolGetDatum(pubactions.pubdelete); + values[Anum_pg_publication_pubdelete - 1] = + BoolGetDatum(pubactions & PUBACTION_DELETE); replaces[Anum_pg_publication_pubdelete - 1] = true; - values[Anum_pg_publication_pubtruncate - 1] = BoolGetDatum(pubactions.pubtruncate); + values[Anum_pg_publication_pubtruncate - 1] = + BoolGetDatum(pubactions & PUBACTION_TRUNCATE); replaces[Anum_pg_publication_pubtruncate - 1] = true; } diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 574d7d2..4e2f9d5 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -567,7 +567,7 @@ ExecSimpleRelationDelete(ResultRelInfo *resultRelInfo, void CheckCmdReplicaIdentity(Relation rel, CmdType cmd) { - PublicationActions *pubactions; + PublicationActions pubactions; /* We only need to do checks for UPDATE and DELETE. */ if (cmd != CMD_UPDATE && cmd != CMD_DELETE) @@ -584,13 +584,13 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd) * Check if the table publishes UPDATES or DELETES. */ pubactions = GetRelationPublicationActions(rel); - if (cmd == CMD_UPDATE && pubactions->pubupdate) + if (cmd == CMD_UPDATE && (pubactions & PUBACTION_UPDATE)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot update table \"%s\" because it does not have a replica identity and publishes updates", RelationGetRelationName(rel)), errhint("To enable updating the table, set REPLICA IDENTITY using ALTER TABLE."))); - else if (cmd == CMD_DELETE && pubactions->pubdelete) + else if (cmd == CMD_DELETE && (pubactions & PUBACTION_DELETE)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot delete from table \"%s\" because it does not have a replica identity and publishes deletes", diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 6f6a203..11a0277 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -653,15 +653,15 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, switch (change->action) { case REORDER_BUFFER_CHANGE_INSERT: - if (!relentry->pubactions.pubinsert) + if (!(relentry->pubactions & PUBACTION_INSERT)) return; break; case REORDER_BUFFER_CHANGE_UPDATE: - if (!relentry->pubactions.pubupdate) + if (!(relentry->pubactions & PUBACTION_UPDATE)) return; break; case REORDER_BUFFER_CHANGE_DELETE: - if (!relentry->pubactions.pubdelete) + if (!(relentry->pubactions & PUBACTION_DELETE)) return; break; default: @@ -796,7 +796,7 @@ pgoutput_truncate(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, relentry = get_rel_sync_entry(data, relid); - if (!relentry->pubactions.pubtruncate) + if (!(relentry->pubactions & PUBACTION_TRUNCATE)) continue; /* @@ -1139,8 +1139,7 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) entry->schema_sent = false; entry->streamed_txns = NIL; entry->replicate_valid = false; - entry->pubactions.pubinsert = entry->pubactions.pubupdate = - entry->pubactions.pubdelete = entry->pubactions.pubtruncate = false; + entry->pubactions = 0; entry->publish_as_relid = InvalidOid; entry->map = NULL; /* will be set by maybe_send_schema() if * needed */ @@ -1239,14 +1238,10 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) if (publish && (relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)) { - entry->pubactions.pubinsert |= pub->pubactions.pubinsert; - entry->pubactions.pubupdate |= pub->pubactions.pubupdate; - entry->pubactions.pubdelete |= pub->pubactions.pubdelete; - entry->pubactions.pubtruncate |= pub->pubactions.pubtruncate; + entry->pubactions |= pub->pubactions; } - if (entry->pubactions.pubinsert && entry->pubactions.pubupdate && - entry->pubactions.pubdelete && entry->pubactions.pubtruncate) + if (entry->pubactions == PUBACTION_ALL) break; } @@ -1387,10 +1382,7 @@ rel_sync_cache_publication_cb(Datum arg, int cacheid, uint32 hashvalue) * There might be some relations dropped from the publication so we * don't need to publish the changes for them. */ - entry->pubactions.pubinsert = false; - entry->pubactions.pubupdate = false; - entry->pubactions.pubdelete = false; - entry->pubactions.pubtruncate = false; + entry->pubactions = 0; } } diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 105d8d4..fed94aa 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -2418,8 +2418,7 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc) bms_free(relation->rd_pkattr); bms_free(relation->rd_idattr); bms_free(relation->rd_hotblockingattr); - if (relation->rd_pubactions) - pfree(relation->rd_pubactions); + relation->rd_pubactions = 0; if (relation->rd_options) pfree(relation->rd_options); if (relation->rd_indextuple) @@ -5524,14 +5523,13 @@ RelationGetExclusionInfo(Relation indexRelation, /* * Get publication actions for the given relation. */ -struct PublicationActions * +PublicationActions GetRelationPublicationActions(Relation relation) { List *puboids; ListCell *lc; - MemoryContext oldcxt; Oid schemaid; - PublicationActions *pubactions = palloc0(sizeof(PublicationActions)); + PublicationActions pubactions = 0; /* * If not publishable, it publishes no actions. (pgoutput_change() will @@ -5540,9 +5538,8 @@ GetRelationPublicationActions(Relation relation) if (!is_publishable_relation(relation)) return pubactions; - if (relation->rd_pubactions) - return memcpy(pubactions, relation->rd_pubactions, - sizeof(PublicationActions)); + if (relation->rd_pubactions & PUBACTION_IN_RELCACHE) + return relation->rd_pubactions & PUBACTION_ALL; /* Fetch the publication membership info. */ puboids = GetRelationPublications(RelationGetRelid(relation)); @@ -5581,10 +5578,10 @@ GetRelationPublicationActions(Relation relation) pubform = (Form_pg_publication) GETSTRUCT(tup); - pubactions->pubinsert |= pubform->pubinsert; - pubactions->pubupdate |= pubform->pubupdate; - pubactions->pubdelete |= pubform->pubdelete; - pubactions->pubtruncate |= pubform->pubtruncate; + if (pubform->pubinsert) pubactions |= PUBACTION_INSERT; + if (pubform->pubupdate) pubactions |= PUBACTION_UPDATE; + if (pubform->pubdelete) pubactions |= PUBACTION_DELETE; + if (pubform->pubtruncate) pubactions |= PUBACTION_TRUNCATE; ReleaseSysCache(tup); @@ -5592,22 +5589,12 @@ GetRelationPublicationActions(Relation relation) * If we know everything is replicated, there is no point to check for * other publications. */ - if (pubactions->pubinsert && pubactions->pubupdate && - pubactions->pubdelete && pubactions->pubtruncate) + if (pubactions == PUBACTION_ALL) break; } - if (relation->rd_pubactions) - { - pfree(relation->rd_pubactions); - relation->rd_pubactions = NULL; - } - - /* Now save copy of the actions in the relcache entry. */ - oldcxt = MemoryContextSwitchTo(CacheMemoryContext); - relation->rd_pubactions = palloc(sizeof(PublicationActions)); - memcpy(relation->rd_pubactions, pubactions, sizeof(PublicationActions)); - MemoryContextSwitchTo(oldcxt); + /* Now save the actions in the relcache entry. */ + relation->rd_pubactions = pubactions | PUBACTION_IN_RELCACHE; return pubactions; } @@ -6162,7 +6149,7 @@ load_relcache_init_file(bool shared) rel->rd_pkattr = NULL; rel->rd_idattr = NULL; rel->rd_hotblockingattr = NULL; - rel->rd_pubactions = NULL; + rel->rd_pubactions = 0; rel->rd_statvalid = false; rel->rd_statlist = NIL; rel->rd_fkeyvalid = false; diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 902f2f2..a8b98ed 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -66,13 +66,13 @@ typedef FormData_pg_publication *Form_pg_publication; DECLARE_UNIQUE_INDEX_PKEY(pg_publication_oid_index, 6110, PublicationObjectIndexId, on pg_publication using btree(oid oid_ops)); DECLARE_UNIQUE_INDEX(pg_publication_pubname_index, 6111, PublicationNameIndexId, on pg_publication using btree(pubname name_ops)); -typedef struct PublicationActions -{ - bool pubinsert; - bool pubupdate; - bool pubdelete; - bool pubtruncate; -} PublicationActions; +#define PUBACTION_INSERT (1 << 0) +#define PUBACTION_UPDATE (1 << 1) +#define PUBACTION_DELETE (1 << 2) +#define PUBACTION_TRUNCATE (1 << 3) +#define PUBACTION_IN_RELCACHE (1 << 4) +#define PUBACTION_ALL (PUBACTION_INSERT | PUBACTION_UPDATE | PUBACTION_DELETE | PUBACTION_TRUNCATE) +typedef uint8 PublicationActions; typedef struct Publication { diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 3128127..0c4b592 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -161,7 +161,7 @@ typedef struct RelationData Bitmapset *rd_idattr; /* included in replica identity index */ Bitmapset *rd_hotblockingattr; /* cols blocking HOT update */ - PublicationActions *rd_pubactions; /* publication actions */ + PublicationActions rd_pubactions; /* publication actions (valid if PUBACTION_IN_RELCACHE bit set) */ /* * rd_options is set whenever rd_rel is loaded into the relcache entry. diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index 82316bb..1ede853 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -74,8 +74,7 @@ extern void RelationGetExclusionInfo(Relation indexRelation, extern void RelationInitIndexAccessInfo(Relation relation); /* caller must include pg_publication.h */ -struct PublicationActions; -extern struct PublicationActions *GetRelationPublicationActions(Relation relation); +extern uint8 GetRelationPublicationActions(Relation relation); extern void RelationInitTableAccessMethod(Relation relation); -- 1.8.3.1 ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-29 16:30 Justin Pryzby <[email protected]> parent: Peter Smith <[email protected]> 2 siblings, 1 reply; 12+ messages in thread From: Justin Pryzby @ 2021-12-29 16:30 UTC (permalink / raw) To: Peter Smith <[email protected]>; +Cc: [email protected] On Mon, Dec 20, 2021 at 11:18:41AM +1100, Peter Smith wrote: > For some reason the current HEAD PublicationActions is a struct of > boolean representing combinations of the 4 different "publication > actions". > > I felt it is more natural to implement boolean flag combinations using > a bitmask instead of a struct of bools. IMO using the bitmask also > simplifies assignment and checking of said flags. Peter E made a suggestion to use a similar struct with bools last year for REINDEX. https://www.postgresql.org/message-id/[email protected] Alvaro pointed out that the integer flags are better for ABI compatibility - it would allow adding a new flag in backbranches, if that were needed for a bugfix. But that's not very compelling, since the bools have existed in v10... Also, the booleans directly correspond with the catalog. + if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT; This is usually written like: pub->pubactions |= (pubform->pubinsert ? PUBACTION_INSERT : 0) -- Justin ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-30 00:31 Peter Smith <[email protected]> parent: Justin Pryzby <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Peter Smith @ 2021-12-30 00:31 UTC (permalink / raw) To: Justin Pryzby <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Thu, Dec 30, 2021 at 3:30 AM Justin Pryzby <[email protected]> wrote: > > On Mon, Dec 20, 2021 at 11:18:41AM +1100, Peter Smith wrote: > > For some reason the current HEAD PublicationActions is a struct of > > boolean representing combinations of the 4 different "publication > > actions". > > > > I felt it is more natural to implement boolean flag combinations using > > a bitmask instead of a struct of bools. IMO using the bitmask also > > simplifies assignment and checking of said flags. > > Peter E made a suggestion to use a similar struct with bools last year for > REINDEX. > https://www.postgresql.org/message-id/[email protected] > > Alvaro pointed out that the integer flags are better for ABI compatibility - it > would allow adding a new flag in backbranches, if that were needed for a > bugfix. > > But that's not very compelling, since the bools have existed in v10... > Also, the booleans directly correspond with the catalog. > > + if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT; > > This is usually written like: > > pub->pubactions |= (pubform->pubinsert ? PUBACTION_INSERT : 0) > Thanks for the info, I've modified those assignment styles as suggested. ------ Kind Regards, Peter Smith. Fujitsu Australia. Attachments: [application/octet-stream] v3-0001-PublicationActions-use-bit-flags.patch (15.0K, ../../CAHut+PtZ-OxSN_nPohqv389=iRyw78S87nv2XCeF8LwafbQgew@mail.gmail.com/2-v3-0001-PublicationActions-use-bit-flags.patch) download | inline diff: From 5ddd49c4394ae05fae36a07a38f3544619d3b887 Mon Sep 17 00:00:00 2001 From: Peter Smith <[email protected]> Date: Thu, 30 Dec 2021 11:24:31 +1100 Subject: [PATCH v3] PublicationActions - use bit flags. The current PublicationActions is a struct of boolean representing combinations of the 4 different "publication actions". IMO it is more natural to implement these flag combinations using a bitmask instead of a struct of bools. This patch keeps the PublicationActions typedef but changes it to be unsigned int, so the previous structure members are all replaced now by bit operations. The Relation cache is also replaced by a simple PublicationAction bitmask instead of a pointer to an allocated struct of pubaction flags. --- src/backend/catalog/pg_publication.c | 9 ++++--- src/backend/commands/publicationcmds.c | 38 +++++++++++++-------------- src/backend/executor/execReplication.c | 6 ++--- src/backend/replication/pgoutput/pgoutput.c | 24 ++++++----------- src/backend/utils/cache/relcache.c | 40 ++++++++++------------------- src/include/catalog/pg_publication.h | 14 +++++----- src/include/utils/rel.h | 2 +- src/include/utils/relcache.h | 3 +-- 8 files changed, 57 insertions(+), 79 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 62f10bc..d707919 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -783,10 +783,11 @@ GetPublication(Oid pubid) pub->oid = pubid; pub->name = pstrdup(NameStr(pubform->pubname)); pub->alltables = pubform->puballtables; - pub->pubactions.pubinsert = pubform->pubinsert; - pub->pubactions.pubupdate = pubform->pubupdate; - pub->pubactions.pubdelete = pubform->pubdelete; - pub->pubactions.pubtruncate = pubform->pubtruncate; + pub->pubactions = + (pubform->pubinsert ? PUBACTION_INSERT : 0) | + (pubform->pubupdate ? PUBACTION_UPDATE : 0) | + (pubform->pubdelete ? PUBACTION_DELETE : 0) | + (pubform->pubtruncate ? PUBACTION_TRUNCATE : 0); pub->pubviaroot = pubform->pubviaroot; ReleaseSysCache(tup); diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 404bb5d..d509e4b 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -73,10 +73,7 @@ parse_publication_options(ParseState *pstate, *publish_via_partition_root_given = false; /* defaults */ - pubactions->pubinsert = true; - pubactions->pubupdate = true; - pubactions->pubdelete = true; - pubactions->pubtruncate = true; + *pubactions = PUBACTION_ALL; *publish_via_partition_root = false; /* Parse options */ @@ -97,10 +94,7 @@ parse_publication_options(ParseState *pstate, * If publish option was given only the explicitly listed actions * should be published. */ - pubactions->pubinsert = false; - pubactions->pubupdate = false; - pubactions->pubdelete = false; - pubactions->pubtruncate = false; + *pubactions = 0; *publish_given = true; publish = defGetString(defel); @@ -116,13 +110,13 @@ parse_publication_options(ParseState *pstate, char *publish_opt = (char *) lfirst(lc); if (strcmp(publish_opt, "insert") == 0) - pubactions->pubinsert = true; + *pubactions |= PUBACTION_INSERT; else if (strcmp(publish_opt, "update") == 0) - pubactions->pubupdate = true; + *pubactions |= PUBACTION_UPDATE; else if (strcmp(publish_opt, "delete") == 0) - pubactions->pubdelete = true; + *pubactions |= PUBACTION_DELETE; else if (strcmp(publish_opt, "truncate") == 0) - pubactions->pubtruncate = true; + *pubactions |= PUBACTION_TRUNCATE; else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -299,13 +293,13 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(stmt->for_all_tables); values[Anum_pg_publication_pubinsert - 1] = - BoolGetDatum(pubactions.pubinsert); + BoolGetDatum(pubactions & PUBACTION_INSERT); values[Anum_pg_publication_pubupdate - 1] = - BoolGetDatum(pubactions.pubupdate); + BoolGetDatum(pubactions & PUBACTION_UPDATE); values[Anum_pg_publication_pubdelete - 1] = - BoolGetDatum(pubactions.pubdelete); + BoolGetDatum(pubactions & PUBACTION_DELETE); values[Anum_pg_publication_pubtruncate - 1] = - BoolGetDatum(pubactions.pubtruncate); + BoolGetDatum(pubactions & PUBACTION_TRUNCATE); values[Anum_pg_publication_pubviaroot - 1] = BoolGetDatum(publish_via_partition_root); @@ -406,16 +400,20 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, if (publish_given) { - values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactions.pubinsert); + values[Anum_pg_publication_pubinsert - 1] = + BoolGetDatum(pubactions & PUBACTION_INSERT); replaces[Anum_pg_publication_pubinsert - 1] = true; - values[Anum_pg_publication_pubupdate - 1] = BoolGetDatum(pubactions.pubupdate); + values[Anum_pg_publication_pubupdate - 1] = + BoolGetDatum(pubactions & PUBACTION_UPDATE); replaces[Anum_pg_publication_pubupdate - 1] = true; - values[Anum_pg_publication_pubdelete - 1] = BoolGetDatum(pubactions.pubdelete); + values[Anum_pg_publication_pubdelete - 1] = + BoolGetDatum(pubactions & PUBACTION_DELETE); replaces[Anum_pg_publication_pubdelete - 1] = true; - values[Anum_pg_publication_pubtruncate - 1] = BoolGetDatum(pubactions.pubtruncate); + values[Anum_pg_publication_pubtruncate - 1] = + BoolGetDatum(pubactions & PUBACTION_TRUNCATE); replaces[Anum_pg_publication_pubtruncate - 1] = true; } diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 574d7d2..4e2f9d5 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -567,7 +567,7 @@ ExecSimpleRelationDelete(ResultRelInfo *resultRelInfo, void CheckCmdReplicaIdentity(Relation rel, CmdType cmd) { - PublicationActions *pubactions; + PublicationActions pubactions; /* We only need to do checks for UPDATE and DELETE. */ if (cmd != CMD_UPDATE && cmd != CMD_DELETE) @@ -584,13 +584,13 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd) * Check if the table publishes UPDATES or DELETES. */ pubactions = GetRelationPublicationActions(rel); - if (cmd == CMD_UPDATE && pubactions->pubupdate) + if (cmd == CMD_UPDATE && (pubactions & PUBACTION_UPDATE)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot update table \"%s\" because it does not have a replica identity and publishes updates", RelationGetRelationName(rel)), errhint("To enable updating the table, set REPLICA IDENTITY using ALTER TABLE."))); - else if (cmd == CMD_DELETE && pubactions->pubdelete) + else if (cmd == CMD_DELETE && (pubactions & PUBACTION_DELETE)) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot delete from table \"%s\" because it does not have a replica identity and publishes deletes", diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 6f6a203..11a0277 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -653,15 +653,15 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, switch (change->action) { case REORDER_BUFFER_CHANGE_INSERT: - if (!relentry->pubactions.pubinsert) + if (!(relentry->pubactions & PUBACTION_INSERT)) return; break; case REORDER_BUFFER_CHANGE_UPDATE: - if (!relentry->pubactions.pubupdate) + if (!(relentry->pubactions & PUBACTION_UPDATE)) return; break; case REORDER_BUFFER_CHANGE_DELETE: - if (!relentry->pubactions.pubdelete) + if (!(relentry->pubactions & PUBACTION_DELETE)) return; break; default: @@ -796,7 +796,7 @@ pgoutput_truncate(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, relentry = get_rel_sync_entry(data, relid); - if (!relentry->pubactions.pubtruncate) + if (!(relentry->pubactions & PUBACTION_TRUNCATE)) continue; /* @@ -1139,8 +1139,7 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) entry->schema_sent = false; entry->streamed_txns = NIL; entry->replicate_valid = false; - entry->pubactions.pubinsert = entry->pubactions.pubupdate = - entry->pubactions.pubdelete = entry->pubactions.pubtruncate = false; + entry->pubactions = 0; entry->publish_as_relid = InvalidOid; entry->map = NULL; /* will be set by maybe_send_schema() if * needed */ @@ -1239,14 +1238,10 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) if (publish && (relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)) { - entry->pubactions.pubinsert |= pub->pubactions.pubinsert; - entry->pubactions.pubupdate |= pub->pubactions.pubupdate; - entry->pubactions.pubdelete |= pub->pubactions.pubdelete; - entry->pubactions.pubtruncate |= pub->pubactions.pubtruncate; + entry->pubactions |= pub->pubactions; } - if (entry->pubactions.pubinsert && entry->pubactions.pubupdate && - entry->pubactions.pubdelete && entry->pubactions.pubtruncate) + if (entry->pubactions == PUBACTION_ALL) break; } @@ -1387,10 +1382,7 @@ rel_sync_cache_publication_cb(Datum arg, int cacheid, uint32 hashvalue) * There might be some relations dropped from the publication so we * don't need to publish the changes for them. */ - entry->pubactions.pubinsert = false; - entry->pubactions.pubupdate = false; - entry->pubactions.pubdelete = false; - entry->pubactions.pubtruncate = false; + entry->pubactions = 0; } } diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 105d8d4..b80823c 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -2418,8 +2418,7 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc) bms_free(relation->rd_pkattr); bms_free(relation->rd_idattr); bms_free(relation->rd_hotblockingattr); - if (relation->rd_pubactions) - pfree(relation->rd_pubactions); + relation->rd_pubactions = 0; if (relation->rd_options) pfree(relation->rd_options); if (relation->rd_indextuple) @@ -5524,14 +5523,13 @@ RelationGetExclusionInfo(Relation indexRelation, /* * Get publication actions for the given relation. */ -struct PublicationActions * +PublicationActions GetRelationPublicationActions(Relation relation) { List *puboids; ListCell *lc; - MemoryContext oldcxt; Oid schemaid; - PublicationActions *pubactions = palloc0(sizeof(PublicationActions)); + PublicationActions pubactions = 0; /* * If not publishable, it publishes no actions. (pgoutput_change() will @@ -5540,9 +5538,8 @@ GetRelationPublicationActions(Relation relation) if (!is_publishable_relation(relation)) return pubactions; - if (relation->rd_pubactions) - return memcpy(pubactions, relation->rd_pubactions, - sizeof(PublicationActions)); + if (relation->rd_pubactions & PUBACTION_IN_RELCACHE) + return relation->rd_pubactions & PUBACTION_ALL; /* Fetch the publication membership info. */ puboids = GetRelationPublications(RelationGetRelid(relation)); @@ -5581,10 +5578,11 @@ GetRelationPublicationActions(Relation relation) pubform = (Form_pg_publication) GETSTRUCT(tup); - pubactions->pubinsert |= pubform->pubinsert; - pubactions->pubupdate |= pubform->pubupdate; - pubactions->pubdelete |= pubform->pubdelete; - pubactions->pubtruncate |= pubform->pubtruncate; + pubactions |= ( + (pubform->pubinsert ? PUBACTION_INSERT : 0) | + (pubform->pubupdate ? PUBACTION_UPDATE : 0) | + (pubform->pubdelete ? PUBACTION_DELETE : 0) | + (pubform->pubtruncate ? PUBACTION_TRUNCATE : 0)); ReleaseSysCache(tup); @@ -5592,22 +5590,12 @@ GetRelationPublicationActions(Relation relation) * If we know everything is replicated, there is no point to check for * other publications. */ - if (pubactions->pubinsert && pubactions->pubupdate && - pubactions->pubdelete && pubactions->pubtruncate) + if (pubactions == PUBACTION_ALL) break; } - if (relation->rd_pubactions) - { - pfree(relation->rd_pubactions); - relation->rd_pubactions = NULL; - } - - /* Now save copy of the actions in the relcache entry. */ - oldcxt = MemoryContextSwitchTo(CacheMemoryContext); - relation->rd_pubactions = palloc(sizeof(PublicationActions)); - memcpy(relation->rd_pubactions, pubactions, sizeof(PublicationActions)); - MemoryContextSwitchTo(oldcxt); + /* Now save the actions in the relcache entry. */ + relation->rd_pubactions = pubactions | PUBACTION_IN_RELCACHE; return pubactions; } @@ -6162,7 +6150,7 @@ load_relcache_init_file(bool shared) rel->rd_pkattr = NULL; rel->rd_idattr = NULL; rel->rd_hotblockingattr = NULL; - rel->rd_pubactions = NULL; + rel->rd_pubactions = 0; rel->rd_statvalid = false; rel->rd_statlist = NIL; rel->rd_fkeyvalid = false; diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 902f2f2..a8b98ed 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -66,13 +66,13 @@ typedef FormData_pg_publication *Form_pg_publication; DECLARE_UNIQUE_INDEX_PKEY(pg_publication_oid_index, 6110, PublicationObjectIndexId, on pg_publication using btree(oid oid_ops)); DECLARE_UNIQUE_INDEX(pg_publication_pubname_index, 6111, PublicationNameIndexId, on pg_publication using btree(pubname name_ops)); -typedef struct PublicationActions -{ - bool pubinsert; - bool pubupdate; - bool pubdelete; - bool pubtruncate; -} PublicationActions; +#define PUBACTION_INSERT (1 << 0) +#define PUBACTION_UPDATE (1 << 1) +#define PUBACTION_DELETE (1 << 2) +#define PUBACTION_TRUNCATE (1 << 3) +#define PUBACTION_IN_RELCACHE (1 << 4) +#define PUBACTION_ALL (PUBACTION_INSERT | PUBACTION_UPDATE | PUBACTION_DELETE | PUBACTION_TRUNCATE) +typedef uint8 PublicationActions; typedef struct Publication { diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 3128127..0c4b592 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -161,7 +161,7 @@ typedef struct RelationData Bitmapset *rd_idattr; /* included in replica identity index */ Bitmapset *rd_hotblockingattr; /* cols blocking HOT update */ - PublicationActions *rd_pubactions; /* publication actions */ + PublicationActions rd_pubactions; /* publication actions (valid if PUBACTION_IN_RELCACHE bit set) */ /* * rd_options is set whenever rd_rel is loaded into the relcache entry. diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index 82316bb..1ede853 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -74,8 +74,7 @@ extern void RelationGetExclusionInfo(Relation indexRelation, extern void RelationInitIndexAccessInfo(Relation relation); /* caller must include pg_publication.h */ -struct PublicationActions; -extern struct PublicationActions *GetRelationPublicationActions(Relation relation); +extern uint8 GetRelationPublicationActions(Relation relation); extern void RelationInitTableAccessMethod(Relation relation); -- 1.8.3.1 ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: PublicationActions - use bit flags. @ 2021-12-30 00:36 Tom Lane <[email protected]> parent: Peter Smith <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Tom Lane @ 2021-12-30 00:36 UTC (permalink / raw) To: Peter Smith <[email protected]>; +Cc: Justin Pryzby <[email protected]>; PostgreSQL Hackers <[email protected]> Peter Smith <[email protected]> writes: > On Thu, Dec 30, 2021 at 3:30 AM Justin Pryzby <[email protected]> wrote: >> + if (pubform->pubinsert) pub->pubactions |= PUBACTION_INSERT; >> This is usually written like: >> pub->pubactions |= (pubform->pubinsert ? PUBACTION_INSERT : 0) > Thanks for the info, I've modified those assignment styles as suggested. FWIW, I think it's utter nonsense to claim that the second way is preferred over the first. There may be some people who think the second way is more legible, but I don't; and I'm pretty sure that the first way is significantly more common in the PG codebase. regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2021-12-30 00:36 UTC | newest] Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-12-20 00:18 PublicationActions - use bit flags. Peter Smith <[email protected]> 2021-12-20 05:10 ` Greg Nancarrow <[email protected]> 2021-12-20 13:58 ` Peter Eisentraut <[email protected]> 2021-12-20 16:56 ` Tom Lane <[email protected]> 2021-12-20 17:14 ` Alvaro Herrera <[email protected]> 2021-12-21 00:19 ` Greg Nancarrow <[email protected]> 2021-12-21 00:56 ` Tom Lane <[email protected]> 2021-12-21 01:55 ` Greg Nancarrow <[email protected]> 2021-12-29 04:21 ` Peter Smith <[email protected]> 2021-12-29 16:30 ` Justin Pryzby <[email protected]> 2021-12-30 00:31 ` Peter Smith <[email protected]> 2021-12-30 00:36 ` Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox