Horton

Thread

Commits

  1. Re-allow INSERT .. ON CONFLICT DO NOTHING on partitioned tables.

  1. INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-08-02T04:56:16Z

    Starting a new thread for a patch I posted earlier [1] to handle ON
CONFLICT DO NOTHING when inserting into a partitioned table.  It's
intended for PG 11 and so registered in the upcoming CF.

Summary of the previous discussion and the patch for anyone interested:

Currently, if an INSERT statement for a partitioned table mentions the ON
CONFLICT clause, we error out immediately.  It was implemented that way,
because it was thought that it could not be handled with zero support for
defining indexes on partitioned tables.  Peter Geoghegan pointed out [2]
that it's too restrictive a view.

He pointed out that planner doesn't *always* expect indexes to be present
on the table when ON CONFLICT is specified.  They must be present though
if DO UPDATE action is requested, because one would need to also specify
the exact columns on which conflict will be checked and those must covered
by the appropriate indexes.  So, if the table is partitioned and DO UPDATE
is specified, lack of indexes will result in an error saying that a
suitable index is absent.  DO UPDATE action cannot be supported until we
implement the feature to define indexes on partitioned tables.

OTOH, the DO NOTHING case should go through the planner without error,
because neither any columns need to be specified nor any indexes need to
be present covering them.  So, DO NOTHING on partitioned tables might work
after all.  Conflict can only be determined using indexes, which
partitioned tables don't allow, so how?  Leaf partitions into which tuples
are ultimately stored can have indexes defined on them, which can be used
to check for the conflict.

The patch's job is simple:

- Remove the check in the parser that causes an error the moment the
  ON CONFLICT clause is found.

- Fix leaf partition ResultRelInfo initialization code so that the call
  ExecOpenIndices() specifies 'true' for speculative, so that the
  information necessary for conflict checking will be initialized in the
  leaf partition's ResultRelInfo

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/62be3d7a-08f6-5dcb-f5c8-a5b764ca96df%40lab.ntt.co.jp

[2]
https://www.postgresql.org/message-id/CAH2-Wzm10T%2B_PWVM4XO5zaknVbAXkOH9-JW3gRVPm1njLHck_w%40mail.gmail.com

  2. Re: INSERT ON CONFLICT and partitioned tables

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-08-02T10:04:39Z

    I applied the patch on latest master sources and the patch applies cleanly.
The documentation is built without errors.

We do not support following syntax for 'do nothing':

postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b)
do nothing;
ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT
specification

This limitation is because we do not support unique index on partitioned
table.
But, in that sense the following snippet of the documentation seems
misleading:

+       will cause an error if the conflict target is specified (see
+       <xref linkend="sql-insert"> for more details).  That means it's not
+       possible to specify <literal>DO UPDATE</literal> as the alternative
+       action, because it requires the conflict target to be specified.
+       On the other hand, specifying <literal>DO NOTHING</literal> as the
+       alternative action works fine.
May be the last sentence can be rephrased as below:

"On the other hand, specifying <literal>DO NOTHING</literal> without target
as
an alternative action works fine."

Other than this patch looks good to me.

Regards,
Jeevan Ladhe



On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp
> wrote:

> Starting a new thread for a patch I posted earlier [1] to handle ON
> CONFLICT DO NOTHING when inserting into a partitioned table.  It's
> intended for PG 11 and so registered in the upcoming CF.
>
> Summary of the previous discussion and the patch for anyone interested:
>
> Currently, if an INSERT statement for a partitioned table mentions the ON
> CONFLICT clause, we error out immediately.  It was implemented that way,
> because it was thought that it could not be handled with zero support for
> defining indexes on partitioned tables.  Peter Geoghegan pointed out [2]
> that it's too restrictive a view.
>
> He pointed out that planner doesn't *always* expect indexes to be present
> on the table when ON CONFLICT is specified.  They must be present though
> if DO UPDATE action is requested, because one would need to also specify
> the exact columns on which conflict will be checked and those must covered
> by the appropriate indexes.  So, if the table is partitioned and DO UPDATE
> is specified, lack of indexes will result in an error saying that a
> suitable index is absent.  DO UPDATE action cannot be supported until we
> implement the feature to define indexes on partitioned tables.
>
> OTOH, the DO NOTHING case should go through the planner without error,
> because neither any columns need to be specified nor any indexes need to
> be present covering them.  So, DO NOTHING on partitioned tables might work
> after all.  Conflict can only be determined using indexes, which
> partitioned tables don't allow, so how?  Leaf partitions into which tuples
> are ultimately stored can have indexes defined on them, which can be used
> to check for the conflict.
>
> The patch's job is simple:
>
> - Remove the check in the parser that causes an error the moment the
>   ON CONFLICT clause is found.
>
> - Fix leaf partition ResultRelInfo initialization code so that the call
>   ExecOpenIndices() specifies 'true' for speculative, so that the
>   information necessary for conflict checking will be initialized in the
>   leaf partition's ResultRelInfo
>
> Thanks,
> Amit
>
> [1]
> https://www.postgresql.org/message-id/62be3d7a-08f6-5dcb-
> f5c8-a5b764ca96df%40lab.ntt.co.jp
>
> [2]
> https://www.postgresql.org/message-id/CAH2-Wzm10T%2B_PWVM4XO5zaknVbAXkOH9-
> JW3gRVPm1njLHck_w%40mail.gmail.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

  3. Re: INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-08-03T04:24:04Z

    Thanks Jeevan for looking at this.  See comments below.

On 2017/08/02 19:04, Jeevan Ladhe wrote:
> On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote wrote:
>> The patch's job is simple:
>>
>> - Remove the check in the parser that causes an error the moment the
>>   ON CONFLICT clause is found.
>>
>> - Fix leaf partition ResultRelInfo initialization code so that the call
>>   ExecOpenIndices() specifies 'true' for speculative, so that the
>>   information necessary for conflict checking will be initialized in the
>>   leaf partition's ResultRelInfo
>
> I applied the patch on latest master sources and the patch applies cleanly.
> The documentation is built without errors.
>
> We do not support following syntax for 'do nothing':
>
> postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b)
> do nothing;
> ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT
> specification

To nitpick, the above is not a syntax error; we *do* support the syntax to
specify conflict target even when the conflict action is DO NOTHING.  The
error is emitted by the planner when it fails to find the index to cover
column 'b' that's specified as the conflict target.

> This limitation is because we do not support unique index on partitioned
> table.

Yes.

> But, in that sense the following snippet of the documentation seems
> misleading:
>
> +       will cause an error if the conflict target is specified (see
> +       <xref linkend="sql-insert"> for more details).  That means it's not
> +       possible to specify <literal>DO UPDATE</literal> as the alternative
> +       action, because it requires the conflict target to be specified.
> +       On the other hand, specifying <literal>DO NOTHING</literal> as the
> +       alternative action works fine.
> May be the last sentence can be rephrased as below:
>
> "On the other hand, specifying <literal>DO NOTHING</literal> without target
> as
> an alternative action works fine."

I have updated the text.

> Other than this patch looks good to me.

Updated patch attached.

Thanks,
Amit

[1]
https://www.postgresql.org/docs/devel/static/sql-insert.html#sql-on-conflict

  4. Re: INSERT ON CONFLICT and partitioned tables

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2017-08-03T09:07:47Z

    Thanks Amit for addressing the comment.

The patch looks good to me. I have no more comments.
Verified that v2 patch applies cleanly and make check passes.

Thanks,
Jeevan Ladhe

  5. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Simon Riggs <simon@2ndquadrant.com> — 2017-11-19T22:33:14Z

    On 2 August 2017 at 00:56, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

> The patch's job is simple:

Patch no longer applies and has some strange behaviours.

> - Remove the check in the parser that causes an error the moment the
>   ON CONFLICT clause is found.

Where is the check and test that blocks ON CONFLICT UPDATE?

My understanding was the patch was intended to only remove the error
in case of DO NOTHING.

> - Fix leaf partition ResultRelInfo initialization code so that the call
>   ExecOpenIndices() specifies 'true' for speculative, so that the
>   information necessary for conflict checking will be initialized in the
>   leaf partition's ResultRelInfo

Why? There is no caller that needs information.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-11-24T02:45:06Z

    Thank you Simon for the review.

On 2017/11/20 7:33, Simon Riggs wrote:
> On 2 August 2017 at 00:56, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 
>> The patch's job is simple:
> 
> Patch no longer applies and has some strange behaviours.
> 
>> - Remove the check in the parser that causes an error the moment the
>>   ON CONFLICT clause is found.
> 
> Where is the check and test that blocks ON CONFLICT UPDATE?

That check is in transformInsertStmt() and following is the diff from the
patch that removes it:

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 757a4a8fd1..d680d2285c 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -847,16 +847,8 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)

     /* Process ON CONFLICT, if any. */
     if (stmt->onConflictClause)
-    {
-        /* Bail out if target relation is partitioned table */
-        if (pstate->p_target_rangetblentry->relkind ==
RELKIND_PARTITIONED_TABLE)
-            ereport(ERROR,
-                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                     errmsg("ON CONFLICT clause is not supported with
partitioned tables")));
-

ON CONFLICT UPDATE will result in an error because it requires specifying
a conflict_target.  Specifying conflict_target will always result in an
error as things stand today, because planner looks for a constraint/index
covering the same and partitioned tables cannot have one.

> My understanding was the patch was intended to only remove the error
> in case of DO NOTHING.

To be precise, the patch is intended to remove the error for the cases
which don't require specifying conflict_target.  In INSERT's
documentation, conflict_target is described as follows:

"Specifies which conflicts ON CONFLICT takes the alternative action on by
choosing arbiter indexes. Either performs unique index inference, or names
a constraint explicitly. For ON CONFLICT DO NOTHING, it is optional to
specify a conflict_target; when omitted, conflicts with all usable
constraints (and unique indexes) are handled. For ON CONFLICT DO UPDATE, a
conflict_target must be provided."

Note the "For ON CONFLICT DO NOTHING, it is optional to specify a
conflict_target".  That's the case that this patch intends to prevent
error for, that is, the error won't occur if no conflict_target is specified.

>> - Fix leaf partition ResultRelInfo initialization code so that the call
>>   ExecOpenIndices() specifies 'true' for speculative, so that the
>>   information necessary for conflict checking will be initialized in the
>>   leaf partition's ResultRelInfo
> 
> Why? There is no caller that needs information.

It is to be used if and when ExecInsert() calls
ExecCheckIndexConstraints() in the code path to handle ON CONFLICT DO
NOTHING that we're intending to support in some cases.  Note that it will
only check conflicts for the individual leaf partitions using whatever
constraint-enforcing indexes they might have.

Example:

create table foo (a int) partition by list (a);
create table foo123 partition of foo (a unique) for values in (1, 2, 3);
\d foo123
               Table "public.foo123"
 Column |  Type   | Collation | Nullable | Default
=-------+---------+-----------+----------+---------
 a      | integer |           |          |
Partition of: foo FOR VALUES IN (1, 2, 3)
Indexes:
    "foo123_a_key" UNIQUE CONSTRAINT, btree (a)


Without the patch, parser itself will throw an error:

insert into foo values (1)
    on conflict do nothing returning tableoid::regclass, *;
ERROR:  ON CONFLICT clause is not supported with partitioned tables


After applying the patch:

insert into foo values (1)
    on conflict do nothing returning tableoid::regclass, *;
 tableoid | a
=---------+---
 foo123   | 1
(1 row)

INSERT 0 1

insert into foo values (1) on conflict do nothing returning
tableoid::regclass, *;
 tableoid | a
=---------+---
(0 rows)

INSERT 0 0

That worked because no explicit conflict target was specified.  If it is
specified, planner (plancat.c: infer_arbiter_indexes()) will throw an
error, because it cannot find a constraint on foo (which is a partitioned
table).

insert into foo values (1)
    on conflict (a) do nothing returning tableoid::regclass, *;
ERROR:  there is no unique or exclusion constraint matching the ON
CONFLICT specification

We'll hopefully able to support specifying conflict_target after the
Alvaro's work at [1] is finished.

Meanwhile, rebased patch is attached.

Thanks,
Amit

[1] https://commitfest.postgresql.org/15/1365/

  7. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-11-24T02:47:29Z

    On 2017/11/24 11:45, Amit Langote wrote:
> Meanwhile, rebased patch is attached.

Oops, forgot to attach in the last email.  Attached now.

Thanks,
Amit

  8. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-30T04:07:46Z

    On Fri, Nov 24, 2017 at 11:47 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/11/24 11:45, Amit Langote wrote:
>> Meanwhile, rebased patch is attached.
>
> Oops, forgot to attach in the last email.  Attached now.

Moved to next CF.
-- 
Michael

  9. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Robert Haas <robertmhaas@gmail.com> — 2017-11-30T16:02:04Z

    On Wed, Nov 29, 2017 at 11:07 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Nov 24, 2017 at 11:47 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/11/24 11:45, Amit Langote wrote:
>>> Meanwhile, rebased patch is attached.
>>
>> Oops, forgot to attach in the last email.  Attached now.
>
> Moved to next CF.

I have two review comments concerning this patch.  First, I think it
would be a good idea to add this to the regression test, just before
'drop table':

+-- but it works OK if we target the partition directly
+insert into parted_conflict_test_1 values (1) on conflict (b) do
update set a = excluded.a;

Second, this would be the first place where the second argument to
ExecOpenIndices() is passed simply as true.  The only other caller
that doesn't pass constant false is in nodeModifyTable.c and looks
like this:

ExecOpenIndices(resultRelInfo, mtstate->mt_onconflict != ONCONFLICT_NONE);

The intention here appears to be to avoid the overhead of doing
BuildSpeculativeIndexInfo() when it isn't needed.  I think we should
try to do the same thing here.  As written, the patch will do that
work even when the query has no ON CONFLICT clause, which doesn't seem
like a good idea.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  10. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-01T02:01:35Z

    On 2017/12/01 1:02, Robert Haas wrote:
> On Wed, Nov 29, 2017 at 11:07 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Nov 24, 2017 at 11:47 AM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> On 2017/11/24 11:45, Amit Langote wrote:
>>>> Meanwhile, rebased patch is attached.
>>>
>>> Oops, forgot to attach in the last email.  Attached now.
>>
>> Moved to next CF.
> 
> I have two review comments concerning this patch.

Thanks for the review.

>  First, I think it
> would be a good idea to add this to the regression test, just before
> 'drop table':
> 
> +-- but it works OK if we target the partition directly
> +insert into parted_conflict_test_1 values (1) on conflict (b) do
> update set a = excluded.a;

Done.

> Second, this would be the first place where the second argument to
> ExecOpenIndices() is passed simply as true.  The only other caller
> that doesn't pass constant false is in nodeModifyTable.c and looks
> like this:
> 
> ExecOpenIndices(resultRelInfo, mtstate->mt_onconflict != ONCONFLICT_NONE);
> 
> The intention here appears to be to avoid the overhead of doing
> BuildSpeculativeIndexInfo() when it isn't needed.  I think we should
> try to do the same thing here.  As written, the patch will do that
> work even when the query has no ON CONFLICT clause, which doesn't seem
> like a good idea.

Agreed.  One thing though is that ExecSetupPartitionTupleRouting doesn't
receive the mtstate today.  I've a patch pending that will re-design that
interface (for another project) that I will post in the near future, but
meanwhile let's just add a new parameter mtstate so that this patch can
move forward with its business.  The future patch I'm talking about will
keep the parameter intact, so it should be OK now to add the same.

Attached two patches:

0001: refactoring to add the mtstate parameter
0002: the original patch updated to address the above comment

Thanks,
Amit

  11. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Simon Riggs <simon@2ndquadrant.com> — 2017-12-01T02:27:45Z

    On 24 November 2017 at 13:45, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

>> Why? There is no caller that needs information.
>
> It is to be used if and when ExecInsert() calls
> ExecCheckIndexConstraints() in the code path to handle ON CONFLICT DO
> NOTHING that we're intending to support in some cases.  Note that it will
> only check conflicts for the individual leaf partitions using whatever
> constraint-enforcing indexes they might have.

So we should have 2 patches. One for now that does DO NOTHING and
another that adds the change that depends upon Alvaro's work.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-01T04:52:51Z

    On 2017/12/01 11:27, Simon Riggs wrote:
> On 24 November 2017 at 13:45, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 
>>> Why? There is no caller that needs information.
>>
>> It is to be used if and when ExecInsert() calls
>> ExecCheckIndexConstraints() in the code path to handle ON CONFLICT DO
>> NOTHING that we're intending to support in some cases.  Note that it will
>> only check conflicts for the individual leaf partitions using whatever
>> constraint-enforcing indexes they might have.
> 
> So we should have 2 patches. One for now that does DO NOTHING and
> another that adds the change that depends upon Alvaro's work.

Yes, I'd think so.

It won't be until the patch at [1] to add support to define UNIQUE indexes
on partitioned tables is committed that we could support specifying
conflict_target on partitioned tables.  Even then, it would take at least
some executor changes to actually make it work, but at least the planner
won't complain that there are no indexes.  If I try Alvaro's patch today
and see what happens when conflict_target is specified, still get an error
but now it's the executor:

create table p (a int, b char) partition by list (a);
create table p1 partition of p (b unique) for values in (1);

-- on HEAD (before applying the patch on this thread)
insert into p values (1) on conflict (a) do nothing;
ERROR:  ON CONFLICT clause is not supported with partitioned tables

-- after applying the patch on this thread (no indexes yet)
insert into p values (1) on conflict (a) do nothing;
ERROR:  there is no unique or exclusion constraint matching the ON
CONFLICT specification

-- after applying Alvaro's patch at [1]
create unique index on p (a);

-- but, the executor support is missing, so...
insert into p values (1) on conflict (a) do nothing;
ERROR:  unexpected failure to find arbiter index

I will report this on that thread and we can discuss the executor changes
that would be need to make it work there.

Thanks,
Amit

[1] https://commitfest.postgresql.org/16/1365/

  13. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-01T07:44:33Z

    On 2017/12/01 11:01, Amit Langote wrote:
> On 2017/12/01 1:02, Robert Haas wrote:
>> Second, this would be the first place where the second argument to
>> ExecOpenIndices() is passed simply as true.  The only other caller
>> that doesn't pass constant false is in nodeModifyTable.c and looks
>> like this:
>>
>> ExecOpenIndices(resultRelInfo, mtstate->mt_onconflict != ONCONFLICT_NONE);
>>
>> The intention here appears to be to avoid the overhead of doing
>> BuildSpeculativeIndexInfo() when it isn't needed.  I think we should
>> try to do the same thing here.  As written, the patch will do that
>> work even when the query has no ON CONFLICT clause, which doesn't seem
>> like a good idea.
> 
> Agreed.  One thing though is that ExecSetupPartitionTupleRouting doesn't
> receive the mtstate today.  I've a patch pending that will re-design that
> interface (for another project) that I will post in the near future, but
> meanwhile let's just add a new parameter mtstate so that this patch can
> move forward with its business.  The future patch I'm talking about will
> keep the parameter intact, so it should be OK now to add the same.
> 
> Attached two patches:
> 
> 0001: refactoring to add the mtstate parameter
> 0002: the original patch updated to address the above comment

I forgot to consider the fact that mtstate could be NULL in
ExecSetupPartitionTupleRouting(), so would result in dereferencing NULL
pointer when called from CopyFrom(), which fixed in the attached updated
patch.

Thanks,
Amit

  14. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Robert Haas <robertmhaas@gmail.com> — 2017-12-01T17:57:54Z

    On Fri, Dec 1, 2017 at 2:44 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> I forgot to consider the fact that mtstate could be NULL in
> ExecSetupPartitionTupleRouting(), so would result in dereferencing NULL
> pointer when called from CopyFrom(), which fixed in the attached updated
> patch.

a ? b : false can more simply be spelled a && b.

Committed after changing it like that, fixing the broken documentation
build, and making minor edits to the comments and documentation.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  15. Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-07T05:10:45Z

    On 2017/12/02 2:57, Robert Haas wrote:
> On Fri, Dec 1, 2017 at 2:44 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> I forgot to consider the fact that mtstate could be NULL in
>> ExecSetupPartitionTupleRouting(), so would result in dereferencing NULL
>> pointer when called from CopyFrom(), which fixed in the attached updated
>> patch.
> 
> a ? b : false can more simply be spelled a && b.
> 
> Committed after changing it like that, fixing the broken documentation
> build, and making minor edits to the comments and documentation.

Thanks for committing.

Regards,
Amit