Horton
Menu

Re: Column Filtering in Logical Replication

Tomas Vondra <tomas.vondra@enterprisedb.com>

View thread
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Amit Kapila <amit.kapila16@gmail.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-11T04:22:14Z
Lists: pgsql-hackers 
On 3/11/22 03:46, Amit Kapila wrote:
> On Fri, Mar 11, 2022 at 12:44 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> On 3/10/22 04:09, Amit Kapila wrote:
>>> 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.
>>
>> I'm not sure which part of the code does this refer to?
>>
> 
> The below part:
> @@ -464,11 +473,11 @@ logicalrep_write_update(StringInfo out,
> TransactionId xid, Relation rel,
>   pq_sendbyte(out, 'O'); /* old tuple follows */
>   else
>   pq_sendbyte(out, 'K'); /* old key follows */
> - logicalrep_write_tuple(out, rel, oldslot, binary);
> + logicalrep_write_tuple(out, rel, oldslot, binary, columns);
>   }
> 
> I think here instead of columns, the patch needs to send NULL as it is
> already doing in logicalrep_write_delete.
> 

Hmmm, yeah. In practice it doesn't really matter, because NULL means
"send all columns" so it actually relaxes the check. But we only send
the RI keys, which is a subset of the column filter. But will fix.

>>> 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.
>>
>> Yeah, you're right. Will fix.
>>
>>> 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.
>>
>> I agree a shorter-lived context would be better than CacheMemoryContext,
>> but "expr" seems to indicate it's for the expression only, so maybe we
>> should rename that.
>>
> 
> Yeah, we can do that. How about rel_entry_cxt or something like that?
> The idea is that eventually, we should move a few other things of
> RelSyncEntry like attrmap where we are using CacheMemoryContext under
> this context.
> 

Yeah, rel_entry_cxt sounds fine I guess ...

>> But do we really want a memory context for every
>> single entry?
>>
> 
> Any other better idea?
> 

No, I think you're right - it'd be hard/impossible to keep track of all
the memory allocated for expression/estate. It'd be fine for the
columns, because that's just a bitmap, but not for the expressions.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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