Re: row filtering for logical replication
От | Amit Kapila |
---|---|
Тема | Re: row filtering for logical replication |
Дата | |
Msg-id | CAA4eK1K5=FZ47va1NjTrSJADCf91=251LtvqBxNjt4vtZGjPGw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: row filtering for logical replication (Peter Smith <smithpb2250@gmail.com>) |
Ответы |
Re: row filtering for logical replication
(Peter Smith <smithpb2250@gmail.com>)
|
Список | pgsql-hackers |
On Wed, Dec 22, 2021 at 5:26 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Mon, Dec 20, 2021 at 9:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Mon, Dec 20, 2021 at 8:41 AM houzj.fnst@fujitsu.com > > <houzj.fnst@fujitsu.com> wrote: > > > > > > Thanks for the comments, I agree with all the comments. > > > Attach the V49 patch set, which addressed all the above comments on the 0002 > > > patch. > > > > > > > Few comments/suugestions: > > ====================== > > 1. > > + Oid publish_as_relid = InvalidOid; > > + > > + /* > > + * For a partition, if pubviaroot is true, check if any of the > > + * ancestors are published. If so, note down the topmost ancestor > > + * that is published via this publication, the row filter > > + * expression on which will be used to filter the partition's > > + * changes. We could have got the topmost ancestor when collecting > > + * the publication oids, but that will make the code more > > + * complicated. > > + */ > > + if (pubform->pubviaroot && relation->rd_rel->relispartition) > > + { > > + if (pubform->puballtables) > > + publish_as_relid = llast_oid(ancestors); > > + else > > + publish_as_relid = GetTopMostAncestorInPublication(pubform->oid, > > + ancestors); > > + } > > + > > + if (publish_as_relid == InvalidOid) > > + publish_as_relid = relid; > > > > I think you can initialize publish_as_relid as relid and then later > > override it if required. That will save the additional check of > > publish_as_relid. > > > > Fixed in v51* [1] > > > 2. I think your previous version code in GetRelationPublicationActions > > was better as now we have to call memcpy at two places. > > > > Fixed in v51* [1] > > > 3. > > + > > + if (list_member_oid(GetRelationPublications(ancestor), > > + puboid) || > > + list_member_oid(GetSchemaPublications(get_rel_namespace(ancestor)), > > + puboid)) > > + { > > + topmost_relid = ancestor; > > + } > > > > I think here we don't need to use braces ({}) as there is just a > > single statement in the condition. > > > > Fixed in v51* [1] > > > 4. > > +#define IDX_PUBACTION_n 3 > > + ExprState *exprstate[IDX_PUBACTION_n]; /* ExprState array for row filter. > > + One per publication action. */ > > .. > > .. > > > > I think we can have this define outside the structure. I don't like > > this define name, can we name it NUM_ROWFILTER_TYPES or something like > > that? > > > > Partly fixed in v51* [1], I've changed the #define name but I did not > move it. The adjacent comment talks about these ExprState caches and > explains the reason why the number is 3. So if I move the #define then > half that comment would have to move with it. I thought it is better > to keep all the related parts grouped together with the one > explanatory comment, but if you still want the #define moved please > confirm and I can do it in a future version. > Yeah, I would prefer it to be moved. You can move the part of the comment suggesting three pubactions can be used for row filtering. -- With Regards, Amit Kapila.
В списке pgsql-hackers по дате отправления:
Предыдущее
От: "houzj.fnst@fujitsu.com"Дата:
Сообщение: RE: Delay the variable initialization in get_rel_sync_entry