From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> |
Cc: | Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: adding partitioned tables to publications |
Date: | 2020-03-27 13:10:28 |
Message-ID: | CA+HiwqFjYE5anArxvkjr37AQMd52L-LZtz9Ld2QrLQ3YfcYhTw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Mar 26, 2020 at 11:23 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Mar 25, 2020 at 9:29 PM Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> > On 2020-03-23 06:02, Amit Langote wrote:
> > > Okay, added some tests.
> > >
> > > Attached updated patches.
> >
> > I have committed the worker.c refactoring patch.
> >
> > "Add subscription support to replicate into partitioned tables" still
> > has lacking test coverage. Your changes in relation.c are not exercised
> > at all because the partitioned table branch in apply_handle_update() is
> > never taken. This is critical and tricky code, so I would look for
> > significant testing.
>
> While trying some tests around the code you mentioned, I found what
> looks like a bug, which looking into now.
Turns out the code in apply_handle_tuple_routing() for the UPDATE
message was somewhat bogus, which fixed in the updated version. I
ended up with anothing refactoring patch, which attached as 0001.
It appears to me that the tests now seem enough to cover
apply_handle_tuple_routing(), although more could still be added.
> > The code looks okay to me. I would remove this code
> >
> > + memset(entry->attrmap->attnums, -1,
> > + entry->attrmap->maplen * sizeof(AttrNumber));
> >
> > because the entries are explicitly filled right after anyway, and
> > filling the bytes with -1 has an unclear effect. There is also
> > seemingly some fishiness in this code around whether attribute numbers
> > are zero- or one-based. Perhaps this could be documented briefly.
> > Maybe I'm misunderstanding something.
>
> Will check and fix as necessary.
Removed that memset. I have added a comment about one- vs. zero-based
indexes contained in the maps coming from two different modules, viz.
tuple routing and logical replication, resp.
--
Thank you,
Amit Langote
EnterpriseDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
v14-0003-Publish-partitioned-table-inserts-as-its-own.patch | application/octet-stream | 60.8 KB |
v1-0001-worker.c-refactor-code-to-look-up-local-tuple.patch | application/octet-stream | 4.3 KB |
v14-0002-Add-subscription-support-to-replicate-into-parti.patch | application/octet-stream | 24.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Julien Rouhaud | 2020-03-27 14:27:13 | Re: Planning counters in pg_stat_statements (using pgss_store) |
Previous Message | Muhammad Usama | 2020-03-27 13:06:14 | Re: Transactions involving multiple postgres foreign servers, take 2 |