Bugs in pgoutput.c
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Date: 2022-01-05T22:11:54Z
Lists: pgsql-hackers
Attachments
- clean-up-pgoutput-cache-invalidation.patch (text/x-diff) patch
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. The attached patch fixes these things, but I'm still pretty unhappy with the general design of the data structures in pgoutput.c, because there is this weird random mishmash of static variables along with a palloc'd PGOutputData struct. This cannot work if there are ever two active LogicalDecodingContexts in the same process. I don't think serial use of LogicalDecodingContexts (ie, destroy one and then make another) works very well either, because pgoutput_shutdown is a mere fig leaf that ignores all the junk the module previously made (in CacheMemoryContext no less). So I wonder whether either of those scenarios is possible/supported/ expected to be needed in future. Also ... maybe I'm not looking in the right place, but I do not see anything anywhere in logical decoding that is taking any lock on the relation being processed. How can that be safe? In particular, how do we know that the data collected by get_rel_sync_entry isn't already stale by the time we return from the function? Setting replicate_valid = true at the bottom of the function would overwrite any notification we might have gotten from a syscache callback while reading catalog data. regards, tom lane
Commits
-
Improve invalidation handling in pgoutput.c.
- 7f481b8d3884 15.0 landed
-
Reduce relcache access in WAL sender streaming logical changes
- 6ce16088bfed 15.0 cited