Thread: pgsql: Fix assorted missing infrastructure for ON CONFLICT.

pgsql: Fix assorted missing infrastructure for ON CONFLICT.

From
Tom Lane
Date:
Fix assorted missing infrastructure for ON CONFLICT.

subquery_planner() failed to apply expression preprocessing to the
arbiterElems and arbiterWhere fields of an OnConflictExpr.  No doubt the
theory was that this wasn't necessary because we don't actually try to
execute those expressions; but that's wrong, because it results in failure
to match to index expressions or index predicates that are changed at all
by preprocessing.  Per bug #14132 from Reynold Smith.

Also add pullup_replace_vars processing for onConflictWhere.  Perhaps
it's impossible to have a subquery reference there, but I'm not exactly
convinced; and even if true today it's a failure waiting to happen.

Also add some comments to other places where one or another field of
OnConflictExpr is intentionally ignored, with explanation as to why it's
okay to do so.

Also, catalog/dependency.c failed to record any dependency on the named
constraint in ON CONFLICT ON CONSTRAINT, allowing such a constraint to
be dropped while rules exist that depend on it, and allowing pg_dump to
dump such a rule before the constraint it refers to.  The normal execution
path managed to error out reasonably for a dangling constraint reference,
but ruleutils.c dumped core; so in addition to fixing the omission, add
a protective check in ruleutils.c, since we can't retroactively add a
dependency in existing databases.

Back-patch to 9.5 where this code was introduced.

Report: <20160510190350.2608.48667@wrigleys.postgresql.org>

Branch
------
REL9_5_STABLE

Details
-------
http://git.postgresql.org/pg/commitdiff/58d802410ad85c44073d4ef494a9d5ac24ecba66

Modified Files
--------------
src/backend/catalog/dependency.c              |  9 +++++++++
src/backend/optimizer/plan/planner.c          | 17 ++++++++++++++---
src/backend/optimizer/plan/subselect.c        |  1 +
src/backend/optimizer/prep/prepjointree.c     | 26 ++++++++++++++++++++++++--
src/backend/optimizer/util/plancat.c          |  9 ++++-----
src/backend/utils/adt/ruleutils.c             |  7 +++++--
src/test/regress/expected/insert_conflict.out | 15 +++++++++++++++
src/test/regress/sql/insert_conflict.sql      | 22 ++++++++++++++++++++++
8 files changed, 94 insertions(+), 12 deletions(-)


Re: pgsql: Fix assorted missing infrastructure for ON CONFLICT.

From
Peter Geoghegan
Date:
On Wed, May 11, 2016 at 1:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Fix assorted missing infrastructure for ON CONFLICT.

What does this mean?

+           /* XXX broken */
            if (attno < 0)
                elog(ERROR, "system column in index");

--
Peter Geoghegan


Re: pgsql: Fix assorted missing infrastructure for ON CONFLICT.

From
Alvaro Herrera
Date:
Peter Geoghegan wrote:
> On Wed, May 11, 2016 at 1:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Fix assorted missing infrastructure for ON CONFLICT.
>
> What does this mean?
>
> +           /* XXX broken */
>             if (attno < 0)
>                 elog(ERROR, "system column in index");

My guess is that it means we do support indexes in system columns (oid
in particular) and that instead of an ugly error this should do
something else.  Maybe silently ignore the index.

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


Re: pgsql: Fix assorted missing infrastructure for ON CONFLICT.

From
Peter Geoghegan
Date:
On Wed, May 11, 2016 at 1:54 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> My guess is that it means we do support indexes in system columns (oid
> in particular) and that instead of an ugly error this should do
> something else.  Maybe silently ignore the index.

Why ignore the index? Either they're not supported, and we should
throw an error (granted, a less ugly one), or they are supported, and
inference should succeed.

--
Peter Geoghegan


Re: pgsql: Fix assorted missing infrastructure for ON CONFLICT.

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Peter Geoghegan wrote:
>> What does this mean?
>>
>> +           /* XXX broken */
>> if (attno < 0)
>> elog(ERROR, "system column in index");

> My guess is that it means we do support indexes in system columns (oid
> in particular) and that instead of an ugly error this should do
> something else.  Maybe silently ignore the index.

I left that for a second patch, which is now pushed.

            regards, tom lane