public inbox for [email protected]help / color / mirror / Atom feed
Re: Bugs in pgoutput.c 5+ messages / 4 participants [nested] [flat]
* Re: Bugs in pgoutput.c @ 2022-02-03 02:45 Amit Kapila <[email protected]> 2022-02-03 02:48 ` Re: Bugs in pgoutput.c Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Amit Kapila @ 2022-02-03 02:45 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Hou, Zhijie/侯 志杰 <[email protected]> On Sat, Jan 29, 2022 at 8:32 AM Amit Kapila <[email protected]> wrote: > > On Thu, Jan 6, 2022 at 3:42 AM Tom Lane <[email protected]> wrote: > > > > Commit 6ce16088b caused me to look at pgoutput.c's handling of > > cache invalidations, and I was pretty appalled by what I found. > > > > * rel_sync_cache_relation_cb does the wrong thing when called for > > a cache flush (i.e., relid == 0). Instead of invalidating all > > RelationSyncCache entries as it should, it will do nothing. > > > > * When rel_sync_cache_relation_cb does invalidate an entry, > > it immediately zaps the entry->map structure, even though that > > might still be in use (as per the adjacent comment that carefully > > explains why this isn't safe). I'm not sure if this could lead > > to a dangling-pointer core dump, but it sure seems like it could > > lead to failing to translate tuples that are about to be sent. > > > > * Similarly, rel_sync_cache_publication_cb is way too eager to > > reset the pubactions flags, which would likely lead to failing > > to transmit changes that we should transmit. > > > > Are you planning to proceed with this patch? > Tom, is it okay for you if I go ahead with this patch after some testing? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Bugs in pgoutput.c 2022-02-03 02:45 Re: Bugs in pgoutput.c Amit Kapila <[email protected]> @ 2022-02-03 02:48 ` Tom Lane <[email protected]> 2022-02-03 11:54 ` Re: Bugs in pgoutput.c Amit Kapila <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2022-02-03 02:48 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Hou, Zhijie/侯 志杰 <[email protected]> Amit Kapila <[email protected]> writes: > Tom, is it okay for you if I go ahead with this patch after some testing? I've been too busy to get back to it, so sure. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Bugs in pgoutput.c 2022-02-03 02:45 Re: Bugs in pgoutput.c Amit Kapila <[email protected]> 2022-02-03 02:48 ` Re: Bugs in pgoutput.c Tom Lane <[email protected]> @ 2022-02-03 11:54 ` Amit Kapila <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Amit Kapila @ 2022-02-03 11:54 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Hou, Zhijie/侯 志杰 <[email protected]> On Thu, Feb 3, 2022 at 8:18 AM Tom Lane <[email protected]> wrote: > > Amit Kapila <[email protected]> writes: > > Tom, is it okay for you if I go ahead with this patch after some testing? > > I've been too busy to get back to it, so sure. > Thanks. I have tested the patch by generating an invalidation message for table DDL before accessing the syscache in logicalrep_write_tuple(). I see that it correctly invalidates the entry and rebuilds it for the next operation. I couldn't come up with some automatic test for it so used the debugger to test it. I have made a minor change in one of the comments. I am planning to push this tomorrow unless there are comments or suggestions. -- With Regards, Amit Kapila. Attachments: [application/octet-stream] v2-0001-Improve-invalidation-handling-in-pgoutput.c.patch (7.8K, ../../CAA4eK1KYdLj7yhN858SxLrwbtxtre1d=z94bpibZWwueYuHCnw@mail.gmail.com/2-v2-0001-Improve-invalidation-handling-in-pgoutput.c.patch) download | inline diff: From 91860283e0a5f6bca9e242c5be3162b4361751ca Mon Sep 17 00:00:00 2001 From: Amit Kapila <[email protected]> Date: Thu, 3 Feb 2022 14:55:37 +0530 Subject: [PATCH v2] Improve invalidation handling in pgoutput.c. Fix the following issues in pgoutput.c: * rel_sync_cache_relation_cb does the wrong thing when called for a cache flush (i.e., relid == 0). Instead of invalidating all RelationSyncCache entries as it should, it does nothing. * When rel_sync_cache_relation_cb does invalidate an entry, it immediately zaps the entry->map structure, even though that might still be in use. We instead just mark the entry as invalid and rebuild it at a later safe point. * Similarly, rel_sync_cache_publication_cb is way too eager to reset the pubactions flags, which would likely lead to failing to transmit changes that we should transmit. In this case also, we just mark the entry as invalid and rebuild it at a later safe point. Author: Tom Lane Reviewed-by: Amit Kapila Discussion: https://postgr.es/m/[email protected] --- src/backend/replication/pgoutput/pgoutput.c | 115 ++++++++++++-------- 1 file changed, 68 insertions(+), 47 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index af8d51aee9..6df705f90f 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -108,11 +108,13 @@ typedef struct RelationSyncEntry { Oid relid; /* relation oid */ + bool replicate_valid; /* overall validity flag for entry */ + bool schema_sent; List *streamed_txns; /* streamed toplevel transactions with this * schema */ - bool replicate_valid; + /* are we publishing this rel? */ PublicationActions pubactions; /* @@ -903,7 +905,9 @@ LoadPublications(List *pubnames) } /* - * Publication cache invalidation callback. + * Publication syscache invalidation callback. + * + * Called for invalidations on pg_publication. */ static void publication_invalidation_cb(Datum arg, int cacheid, uint32 hashvalue) @@ -1130,13 +1134,12 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) HASH_ENTER, &found); Assert(entry != NULL); - /* Not found means schema wasn't sent */ + /* initialize entry, if it's new */ if (!found) { - /* immediately make a new entry valid enough to satisfy callbacks */ + entry->replicate_valid = false; 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->publish_as_relid = InvalidOid; @@ -1166,13 +1169,40 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) { oldctx = MemoryContextSwitchTo(CacheMemoryContext); if (data->publications) + { list_free_deep(data->publications); - + data->publications = NIL; + } data->publications = LoadPublications(data->publication_names); MemoryContextSwitchTo(oldctx); publications_valid = true; } + /* + * Reset schema_sent status as the relation definition may have + * changed. Also reset pubactions to empty in case rel was dropped + * from a publication. Also free any objects that depended on the + * earlier definition. + */ + entry->schema_sent = false; + list_free(entry->streamed_txns); + entry->streamed_txns = NIL; + entry->pubactions.pubinsert = false; + entry->pubactions.pubupdate = false; + entry->pubactions.pubdelete = false; + entry->pubactions.pubtruncate = false; + if (entry->map) + { + /* + * Must free the TupleDescs contained in the map explicitly, + * because free_conversion_map() doesn't. + */ + FreeTupleDesc(entry->map->indesc); + FreeTupleDesc(entry->map->outdesc); + free_conversion_map(entry->map); + } + entry->map = NULL; + /* * Build publication cache. We can't use one provided by relcache as * relcache considers all publications given relation is in, but here @@ -1212,16 +1242,18 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) foreach(lc2, ancestors) { Oid ancestor = lfirst_oid(lc2); + List *apubids = GetRelationPublications(ancestor); + List *aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor)); - if (list_member_oid(GetRelationPublications(ancestor), - pub->oid) || - list_member_oid(GetSchemaPublications(get_rel_namespace(ancestor)), - pub->oid)) + if (list_member_oid(apubids, pub->oid) || + list_member_oid(aschemaPubids, pub->oid)) { ancestor_published = true; if (pub->pubviaroot) publish_as_relid = ancestor; } + list_free(apubids); + list_free(aschemaPubids); } } @@ -1251,6 +1283,7 @@ get_rel_sync_entry(PGOutputData *data, Oid relid) } list_free(pubids); + list_free(schemaPubids); entry->publish_as_relid = publish_as_relid; entry->replicate_valid = true; @@ -1322,43 +1355,40 @@ rel_sync_cache_relation_cb(Datum arg, Oid relid) /* * Nobody keeps pointers to entries in this hash table around outside * logical decoding callback calls - but invalidation events can come in - * *during* a callback if we access the relcache in the callback. Because - * of that we must mark the cache entry as invalid but not remove it from - * the hash while it could still be referenced, then prune it at a later - * safe point. - * - * Getting invalidations for relations that aren't in the table is - * entirely normal, since there's no way to unregister for an invalidation - * event. So we don't care if it's found or not. + * *during* a callback if we do any syscache access in the callback. + * Because of that we must mark the cache entry as invalid but not damage + * any of its substructure here. The next get_rel_sync_entry() call will + * rebuild it all. */ - entry = (RelationSyncEntry *) hash_search(RelationSyncCache, &relid, - HASH_FIND, NULL); - - /* - * Reset schema sent status as the relation definition may have changed. - * Also free any objects that depended on the earlier definition. - */ - if (entry != NULL) + if (OidIsValid(relid)) { - entry->schema_sent = false; - list_free(entry->streamed_txns); - entry->streamed_txns = NIL; - if (entry->map) + /* + * Getting invalidations for relations that aren't in the table is + * entirely normal. So we don't care if it's found or not. + */ + entry = (RelationSyncEntry *) hash_search(RelationSyncCache, &relid, + HASH_FIND, NULL); + if (entry != NULL) + entry->replicate_valid = false; + } + else + { + /* Whole cache must be flushed. */ + HASH_SEQ_STATUS status; + + hash_seq_init(&status, RelationSyncCache); + while ((entry = (RelationSyncEntry *) hash_seq_search(&status)) != NULL) { - /* - * Must free the TupleDescs contained in the map explicitly, - * because free_conversion_map() doesn't. - */ - FreeTupleDesc(entry->map->indesc); - FreeTupleDesc(entry->map->outdesc); - free_conversion_map(entry->map); + entry->replicate_valid = false; } - entry->map = NULL; } } /* * Publication relation/schema map syscache invalidation callback + * + * Called for invalidations on pg_publication, pg_publication_rel, and + * pg_publication_namespace. */ static void rel_sync_cache_publication_cb(Datum arg, int cacheid, uint32 hashvalue) @@ -1382,15 +1412,6 @@ rel_sync_cache_publication_cb(Datum arg, int cacheid, uint32 hashvalue) while ((entry = (RelationSyncEntry *) hash_seq_search(&status)) != NULL) { entry->replicate_valid = false; - - /* - * 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; } } -- 2.28.0.windows.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: hang during shutdown @ 2026-07-10 05:19 Jeremy Schneider <[email protected]> 2026-07-10 06:49 ` Re: hang during shutdown Anthonin Bonnefoy <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Jeremy Schneider @ 2026-07-10 05:19 UTC (permalink / raw) To: Anthonin Bonnefoy <[email protected]>; +Cc: Jakub Wartak <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; pgsql-hackers On Thu, 9 Jul 2026 09:09:25 +0200 Anthonin Bonnefoy <[email protected]> wrote: > As you didn't have a shutdown checkpoint, the symptoms look very > similar (though you didn't confirm whether you were using logical > replication, so I'm gonna assume you do). No logical replication. Are we sure this only impacts logical and not physical? Thanks for the pointer - interesting one. We do use physical replication. Replicas were shut down and they closed their connections to this writer instance around 04:25 just before pg_ctl stop was executed on the writer itself. Something was on the writer DB's CPU before shutdown; metrics show total CPU for that linux namespace (k8s pod) around 0.86 cores until 04:24 and then the CPU starts dropping when the database shuts down. Bu 04:28 the CPU is under 0.01 cores and the CPU remains effectively zero until the processes are SIGKILL'd 30 minutes later. -Jeremy -- To know the thoughts and deeds that have marked man's progress is to feel the great heart throbs of humanity through the centuries; and if one does not feel in these pulsations a heavenward striving, one must indeed be deaf to the harmonies of life. Helen Keller, The Story Of My Life, 1902, 1903, 1905, introduction by Ralph Barton Perry (Garden City, NY: Doubleday & Company, 1954), p90. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: hang during shutdown 2026-07-10 05:19 Re: hang during shutdown Jeremy Schneider <[email protected]> @ 2026-07-10 06:49 ` Anthonin Bonnefoy <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Anthonin Bonnefoy @ 2026-07-10 06:49 UTC (permalink / raw) To: Jeremy Schneider <[email protected]>; +Cc: Jakub Wartak <[email protected]>; Srinath Reddy Sadipiralla <[email protected]>; pgsql-hackers On Fri, Jul 10, 2026 at 7:20 AM Jeremy Schneider <[email protected]> wrote: > > On Thu, 9 Jul 2026 09:09:25 +0200 > Anthonin Bonnefoy <[email protected]> wrote: > > > As you didn't have a shutdown checkpoint, the symptoms look very > > similar (though you didn't confirm whether you were using logical > > replication, so I'm gonna assume you do). > > No logical replication. Are we sure this only impacts logical and not > physical? Thanks for the pointer - interesting one. Yeah, this is only for logical replication. If you didn't have one, then it's unrelated. > We do use physical replication. Replicas were shut down and they closed > their connections to this writer instance around 04:25 just before > pg_ctl stop was executed on the writer itself. > > Something was on the writer DB's CPU before shutdown; metrics show total > CPU for that linux namespace (k8s pod) around 0.86 cores until 04:24 and > then the CPU starts dropping when the database shuts down. Bu 04:28 the > CPU is under 0.01 cores and the CPU remains effectively zero until the > processes are SIGKILL'd 30 minutes later. Looking at the last WAL record that was on the whole during shutdown could be interesting information, if you have it available. If the shutdown was stuck due to a specific state of the WAL, it could give you some leads. Other information could be useful, like the types of writes that happened after you started the shutdown, or to confirm whether the RUNNING_XACT you mention is indeed from the autovac and whether it closed the transaction or not. ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-07-10 06:49 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-02-03 02:45 Re: Bugs in pgoutput.c Amit Kapila <[email protected]> 2022-02-03 02:48 ` Tom Lane <[email protected]> 2022-02-03 11:54 ` Amit Kapila <[email protected]> 2026-07-10 05:19 Re: hang during shutdown Jeremy Schneider <[email protected]> 2026-07-10 06:49 ` Re: hang during shutdown Anthonin Bonnefoy <[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