Re: Column Filtering in Logical Replication - Mailing list pgsql-hackers

From Rahila Syed
Subject Re: Column Filtering in Logical Replication
Date
Msg-id CAH2L28sZj2Bj=73qZrXy69y6w5BRKKpunV3P7tnXYQKPbA11gw@mail.gmail.com
Whole thread Raw
In response to Re: Column Filtering in Logical Replication  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
Responses Re: Column Filtering in Logical Replication  (Tomas Vondra <tomas.vondra@enterprisedb.com>)
List pgsql-hackers
Hi Alvaro,

Thank you for comments.

The patch adds a function get_att_num_by_name; but we have a lsyscache.c
function for that purpose, get_attnum.  Maybe that one should be used
instead?

Thank you for pointing that out, I agree it makes sense to reuse the existing function.
Changed it accordingly in the attached patch.
 
get_tuple_columns_map() returns a bitmapset of the attnos of the columns
in the given list, so its name feels wrong.  I propose
get_table_columnset().  However, this function is invoked for every
insert/update change, so it's going to be far too slow to be usable.  I
think you need to cache the bitmapset somewhere, so that the function is
only called on first use.  I didn't look very closely, but it seems that
struct RelationSyncEntry may be a good place to cache it.

Makes sense, changed accordingly.
 
The patch adds a new parse node PublicationTable, but doesn't add
copyfuncs.c, equalfuncs.c, readfuncs.c, outfuncs.c support for it.
Maybe try a compile with WRITE_READ_PARSE_PLAN_TREES and/or
COPY_PARSE_PLAN_TREES enabled to make sure everything is covered.
(I didn't verify that this actually catches anything ...)
 
I will test this and include these changes in the next version.
 
The new column in pg_publication_rel is prrel_attr.  This name seems at
odds with existing column names (we don't use underscores in catalog
column names).  Maybe prattrs is good enough?  prrelattrs?  We tend to
use plurals for columns that are arrays.

Renamed it to prattrs as per suggestion.

It's not super clear to me that strlist_to_textarray() and related
processing will behave sanely when the column names contain weird
characters such as commas or quotes, or just when used with uppercase
column names.  Maybe it's worth having tests that try to break such
cases.

Sure, I will include these tests in the next version.
 
You seem to have left a debugging "elog(LOG)" line in OpenTableList.

Removed.
 
I got warnings from "git am" about trailing whitespace being added by
the patch in two places.

Should be fixed now.

Thank you,
Rahila Syed
 
Attachment

pgsql-hackers by date:

Previous
From: vignesh C
Date:
Subject: Re: Added schema level support for publication.
Next
From: gkokolatos@pm.me
Date:
Subject: Re: Introduce pg_receivewal gzip compression tests