On Jan 14, 2021, at 1:25 PM, Amit Kapila <amit.kapil...@gmail.com<mailto:amit.kapil...@gmail.com>> wrote:
On Wed, Jan 13, 2021 at 5:40 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com<mailto:bharath.rupireddyforpostg...@gmail.com>> wrote: On Wed, Jan 13, 2021 at 3:16 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com<mailto:bharath.rupireddyforpostg...@gmail.com>> wrote: On Wed, Jan 13, 2021 at 2:36 PM japin <japi...@hotmail.com<mailto:japi...@hotmail.com>> wrote: In summary, I feel we need to fix the publisher sending the inserts even though the table is dropped from the publication, that is the patch patch proposed by japin. This solves the bug reported in this thread. And also, it's good to have the LogicalRepRelMap invalidation fix as a 0002 patch in invalidate_syncing_table_states, subscription_change_cb and logicalrep_rel_open as proposed by me. Thoughts? I think invalidate the LogicalRepRelMap is necessary. If the table isn't in subscription, can we remove the LogicalRepRelMapEntry from LogicalRepRelMap? IIUC, it's not possible to know the relid of the alter publication...dropped table in the invalidation callbacks invalidate_syncing_table_states or subscription_change_cb, so it's better to set state of all the entries to SUBREL_STATE_UNKNOWN, so that, in the next logicalrep_rel_open call the function GetSubscriptionRelState will be called and the pg_subscription_rel will be read freshly. This is inline with the reasoning specified at [1]. [1] - https://www.postgresql.org/message-id/CAFiTN-udwuc_h0TaFrSEKb-Yo1vVvkjz9TDRw7VE3P-KiPSGbQ%40mail.gmail.com Attaching following two patches: 0001 - Makes publisher to not publish the changes for the alter publication ... dropped tables. Original patch is by japin, I added comments, changed the function name and adjusted the code a bit. Do we really need to access PUBLICATIONRELMAP in this patch? What if we just set it to false in the else condition of (if (publish && (relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot))) Thank for you review. I agree with you, it doesn’t need to access PUBLICATIONRELMAP, since We already get the publication oid in GetRelationPublications(relid) [1], which also access PUBLICATIONRELMAP. If the current pub->oid does not in pubids, the publish is false, so we do not need publish the table. I have another question, the data->publications is a list, when did it has more then one items? 0001 - change as you suggest. 0002 - does not change. Please consider v2 for further review. [1] List * GetRelationPublications(Oid relid) { List *result = NIL; CatCList *pubrellist; int i; /* Find all publications associated with the relation. */ pubrellist = SearchSysCacheList1(PUBLICATIONRELMAP, ObjectIdGetDatum(relid)); for (i = 0; i < pubrellist->n_members; i++) { HeapTuple tup = &pubrellist->members[i]->tuple; Oid pubid = ((Form_pg_publication_rel) GETSTRUCT(tup))->prpubid; result = lappend_oid(result, pubid); } ReleaseSysCacheList(pubrellist); return result; } -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
v2-0001-Fix-ALTER-PUBLICATION.DROP-TABLE-behaviour.patch
Description: v2-0001-Fix-ALTER-PUBLICATION.DROP-TABLE-behaviour.patch
v2-0002-Invalidate-relation-map-cache-in-subscriber-sysca.patch
Description: v2-0002-Invalidate-relation-map-cache-in-subscriber-sysca.patch
