Re: ON CONFLICT DO UPDATE for partitioned tables

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, Andres Freund <andres(at)anarazel(dot)de>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Peter Geoghegan <pg(at)bowt(dot)ie>
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
Date: 2018-03-26 02:30:51
Message-ID: c8c23eab-4c86-d3ff-f37e-1bc7e0b2a9d8@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2018/03/24 9:23, Alvaro Herrera wrote:
> I made a bunch of further edits and I think this v10 is ready to push.
> Before doing so I'll give it a final look, particularly because of the
> new elog(ERROR) I added. Post-commit review is of course always
> appreciated.
>
> Most notable change is because I noticed that if you mention an
> intermediate partition level in the INSERT command, and the index is on
> the top level, arbiter index selection fails to find the correct index
> because it walks all the way to the top instead of stopping in the
> middle, as it should (the command was still working because it ended up
> with an empty arbiter index list).

Good catch!

> To fix this, I had to completely rework the "get partition parent root"
> stuff into "get list of ancestors of this partition".

I wondered if a is_partition_ancestor(partrelid, ancestorid) isn't enough
instead of creating a list of ancestors and then looping over it as you've
done, but maybe what you have here is fine.

> Because of this, I added a new check that the partition's arbiter index
> list is same length as parent's; if not, throw an error. I couldn't get
> it to fire (so it's just an elog not ereport), but maybe I just didn't
> try any weird enough scenarios.
>
> Other changes:
>
> * I added a copyObject() call for nodes we're operating upon. Maybe
> this is unnecessary but the comments claimed "we're working on a copy"
> and I couldn't find any place where we were actually making one.
> Anyway it seems sane to make a copy, because we're scribbling on those
> nodes ... I hope I didn't introduce any serious memory leaks.

That seems fine as ExecInitPartitionInfo allocates in the query context
(es_query_cxt).

> * I made the new OnConflictSetState thing into a proper node. Like
> ResultRelInfo, it doesn't have any niceties like nodeToString support,
> but it seems saner this way (palloc -> makeNode). I reworked the
> formatting of that struct definition too, and renamed members.

Looks good, thanks.

> * I removed an assertion block at the bottom of adjust_partition_tlist.
> It seemed quite pointless, since it was just about checking that the
> resno values were sorted, but by construction we already know that
> they are indeed sorted ...

Hmm yes.

> * General code style improvements, comment rewording, etc.

There was one comment in Fujita-san's review he posted on Friday [1] that
doesn't seem to be addressed in v10, which I think we probably should. It
was this comment:

"ExecBuildProjectionInfo is called without setting the tuple descriptor of
mtstate->mt_conflproj to tupDesc. That might work at least for now, but I
think it's a good thing to set it appropriately to make that future proof."

All of his other comments seem to have been taken care of in v10. I have
fixed the above one in the attached updated version.

Thanks,
Amit

[1] https://www.postgresql.org/message-id/5AB4DEB6.2020901%40lab.ntt.co.jp

Attachment Content-Type Size
v11-0001-on-conflict.patch text/plain 41.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Craig Ringer 2018-03-26 03:11:08 Re: Proposal: http2 wire format
Previous Message Kyotaro HORIGUCHI 2018-03-26 02:28:41 Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types