Horton
Menu

Re: Column Filtering in Logical Replication

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Justin Pryzby <pryzby@telsasoft.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Rahila Syed <rahilasyed90@gmail.com>, Peter Smith <smithpb2250@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, shiy.fnst@fujitsu.com
Date: 2022-03-10T03:09:46Z
Lists: pgsql-hackers 
On Wed, Mar 9, 2022 at 3:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Mar 7, 2022 at 8:48 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>
> > OK, I reworked this to do the same thing as the row filtering patch.
> >
>
> Thanks, I'll check this.
>

Some assorted comments:
=====================
1. We don't need to send a column list for the old tuple in case of an
update (similar to delete). It is not required to apply a column
filter for those cases because we ensure that RI must be part of the
column list for updates and deletes.
2.
+ /*
+ * Check if all columns referenced in the column filter are part of
+ * the REPLICA IDENTITY index or not.

I think this comment is reverse. The rule we follow here is that
attributes that are part of RI must be there in a specified column
list. This is used at two places in the patch.
3. get_rel_sync_entry()
{
/* XXX is there a danger of memory leak here? beware */
+ oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ for (int i = 0; i < nelems; i++)
...
}

Similar to the row filter, I think we need to use
entry->cache_expr_cxt to allocate this. There are other usages of
CacheMemoryContext in this part of the code but I think those need to
be also changed and we can do that as a separate patch. If we do the
suggested change then we don't need to separately free columns.
4. I think we don't need the DDL changes in AtExecDropColumn. Instead,
we can change the dependency of columns to NORMAL during publication
commands.
5. There is a reference to check_publication_columns but that function
is removed from the patch.
6.
/*
* If we know everything is replicated and the row filter is invalid
* for update and delete, there is no point to check for other
* publications.
*/
if (pubdesc->pubactions.pubinsert && pubdesc->pubactions.pubupdate &&
pubdesc->pubactions.pubdelete && pubdesc->pubactions.pubtruncate &&
!pubdesc->rf_valid_for_update && !pubdesc->rf_valid_for_delete)
break;

/*
* If we know everything is replicated and the column filter is invalid
* for update and delete, there is no point to check for other
* publications.
*/
if (pubdesc->pubactions.pubinsert && pubdesc->pubactions.pubupdate &&
pubdesc->pubactions.pubdelete && pubdesc->pubactions.pubtruncate &&
!pubdesc->cf_valid_for_update && !pubdesc->cf_valid_for_delete)
break;

Can we combine these two checks?

I feel this patch needs a more thorough review.

-- 
With Regards,
Amit Kapila.

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Doc: Explain about Column List feature.

  2. Doc: fix column list vs. replica identity rules.

  3. Prohibit combining publications with different column lists.

  4. Fix the check to limit sync workers.

  5. Wait for subscription to sync in t/031_column_list.sql

  6. Move prattrs to the pg_publication_rel section in docs

  7. Allow specifying column lists for logical replication

  8. Fix row filters with multiple publications

  9. Fix publish_as_relid with multiple publications

  10. Add some additional tests for row filters in logical replication.

  11. Add index on pg_publication_rel.prpubid

  12. Avoid using DefElemAction in AlterPublicationStmt

  13. Small cleanups related to PUBLICATION framework code

  14. Add PublicationTable and PublicationRelInfo structs

  15. Fix various concurrency issues in logical replication worker launching