Re: adding partitioned tables to publications

amit <amitlangote09@gmail.com>

From: Amit Langote <amitlangote09@gmail.com>

To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>

Cc: Rafia Sabih <rafia.pghackers@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>

Date: 2020-03-23T05:02:25Z

Lists: pgsql-hackers

Commits

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

Fix RELCACHE_FORCE_RELEASE issue
- 12fb189bfeaf 13.0 landed
Allow publishing partition changes via ancestors
- 83fd4532a721 13.0 landed
Add logical replication support to replicate into partitioned tables
- f1ac27bfda6c 13.0 landed
Refactor code to look up local replication tuple
- d8653f468789 13.0 landed
Some refactoring of logical/worker.c
- 97ee604d9bce 13.0 landed
Prepare to support non-tables in publications
- c314c147c056 13.0 landed
Support adding partitioned tables to publication
- 17b9e7f9fe23 13.0 landed

Attachments

v13-0003-Publish-partitioned-table-inserts-as-its-own.patch (application/octet-stream) patch v13-0003
v13-0001-Some-refactoring-of-logical-worker.c.patch (application/octet-stream) patch v13-0001
v13-0002-Add-subscription-support-to-replicate-into-parti.patch (application/octet-stream) patch v13-0002

On Thu, Mar 19, 2020 at 11:18 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 2020-03-18 08:33, Amit Langote wrote:
> > By the way, I have rebased the patches, although maybe you've got your
> > own copies; attached.
>
> Looking through 0002 and 0003 now.
>
> The structure looks generally good.

Thanks for the review.

> In 0002, the naming of apply_handle_insert() vs.
> apply_handle_do_insert() etc. seems a bit prone to confusion.  How about
> something like apply_handle_insert_internal()?  Also, should we put each
> of those internal functions next to their internal function instead of
> in a separate group like you have it?

Sure.

> In apply_handle_do_insert(), the argument localslot should probably be
> remoteslot.

You're right, fixed.

> In apply_handle_do_delete(), the ExecOpenIndices() call was moved to a
> different location relative to the rest of the code.  That was probably
> not intended.

Fixed.

> In 0003, you have /* TODO, use inverse lookup hashtable? */.  Is this
> something you plan to address in this cycle, or is that more for future
> generations?

Sorry, this is simply a copy-paste from logicalrep_relmap_invalidate_cb().

> 0003 could use some more tests.  The one test that you adjusted just
> ensures the data goes somewhere instead of being rejected, but there are
> no tests that check whether it ends up in the right partition, whether
> cross-partition updates work etc.

Okay, added some tests.

Attached updated patches.

--
Thank you,
Amit