Skip to content

Routed ON CONFLICT inserts broken by partition-local deferrable unique constraints in 19 and master - #413

Open
pg-hub-mirror[bot] wants to merge 1 commit into
masterfrom
pg-hub/mirror-patch-b5df8733733d78f2
Open

pg-hub-mirror[bot] wants to merge 1 commit into
masterfrom
pg-hub/mirror-patch-b5df8733733d78f2

Conversation

@pg-hub-mirror

@pg-hub-mirror pg-hub-mirror Bot commented Sep 17, 2026

Copy link
Copy Markdown

Read-only mirror. Reply and review on pgsql-hackers; activity here is not sent upstream.

  • Original author: Álvaro Herrera <alvherre(at)kurilemu(dot)de>
  • Mailing list: pgsql-hackers
  • Message-ID: aqwnMDPBLBQ64Eus@alvherre.pgsql
  • Original email

Patch files:


On 2026-Sep-01, Zsolt Parragi wrote:

On Tue, 01 Sep 2026, Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)com> wrote:

This is a "grouped" version. Also, it handles possible collation
issues + provides a set of tests to pin the correct behaviour.

Thanks, this looks better what I had in mind, I would have missed a
few corner cases this patch covers.
I spent some time with this and ended up with the attached. I don't I
found anything to change, apart from minor edits to the commit message.
I'll probably edit it some more before push, to mention the change of
list_difference() to equal().
The non-deterministic collation aspect mentioned in an XXX comment added
by the patch was a bug in 18 and back, and continues to be a bug after
this patch. That's shown with the following test case:
CREATE COLLATION ci (provider = icu, locale = 'und-u-ks-level2', deterministic = false);
-- First part of test case: ON CONFLICT listing a column works fine.
CREATE TABLE t (x text, y text);
ALTER TABLE t ADD CONSTRAINT t_x_key UNIQUE (x);
CREATE UNIQUE INDEX t_x_ci ON t (x COLLATE ci);
INSERT INTO t VALUES ('a', 'first');
INSERT INTO t VALUES ('A', 'second') ON CONFLICT (x) DO UPDATE SET y = excluded.y;
-- the end result here is ('a', 'second'), showing that ON CONFLICT worked.
SELECT x, y FROM t;
-- repeat, but use ON CONFLICT ON CONSTRAINT. Throws error but shouldn't.
INSERT INTO t VALUES ('A', 'third') ON CONFLICT ON CONSTRAINT t_x_key DO UPDATE SET y = excluded.y;
It's not on this patch to solve this problem, as it's not a new problem.
But we should consider a backpatchable fix at some point.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu. Five minutes later I realize that it's also talking
about food" (Donald Knuth)

Commits 2bc7e88 and 90eae92 taught ON CONFLICT to include
indexes matching an already selected arbiter, so that an index left
behind by REINDEX CONCURRENTLY continues to arbitrate together with its
replacement.  Both checks were too permissive:

a) infer_arbiter_indexes() compared a candidate with a named
constraint's index using only attributes, expressions and predicate, but
ignored collation, NULLS NOT DISTINCT setting or deferrability.  As a
result, an index with a difference in these settings could be accepted
even though it did not identify the same conflicts.  Also, a deferrable
index that otherwise matches an arbiter index would also cause ON
CONFLICT to fail with "ON CONFLICT does not support deferrable unique
constraints/exclusion constraints as arbiters".

b) ExecInitPartitionInfo() also failed due to the failure to compare
deferrability, so partition-local deferrable indexes would be considered
and break inserts routed to that partition with the error mentioned
above.

Fix by making IsIndexCompatibleAsArbiter() compare those properties, and
changing infer_arbiter_indexes() to use that routine instead of open
coding equivalent logic.  Also handle the named-constraint case in
infer_arbiter_indexes() separately instead of passing the constraint
index through the regular clause-inference matching.  That function is
not static anymore, so move it to index.c, and also reimplement it to
use the Relation from the indexes only, no longer receiving the
IndexInfo (which wasn't really necessary.)

Add tests for ON CONFLICT ON CONSTRAINT with deferrable, NULLS NOT
DISTINCT, and different-collation sibling indexes, preserving the
behavior of released pre-19 versions.  Also test routed inserts with a
partition-local deferrable unique constraint.

Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAN4CZFPEYXeYFTxHpoPujfVFb+1Tx1jnXVboDMBg-ZhpgpQ-_g@mail.gmail.com
@pg-hub-mirror

pg-hub-mirror Bot commented Sep 17, 2026

Copy link
Copy Markdown
Author

Earlier design discussion: Discussion #377

@pg-hub-mirror pg-hub-mirror Bot added area:storage Storage, access methods, buffers, or I/O source:pgsql-hackers Mirrored from pgsql-hackers type:patch Mail thread contains a PostgreSQL patch area:testing Tests and buildfarm area:sql SQL language or commands labels Sep 17, 2026
@pg-hub-mirror pg-hub-mirror Bot locked and limited conversation to collaborators Sep 17, 2026
@pg-hub-mirror pg-hub-mirror Bot unlocked this conversation Sep 18, 2026
@pg-hub-mirror

pg-hub-mirror Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

Álvaro Herrera <alvherre(at)kurilemu(dot)de> via pgsql-hackers · original email

Pushed, thanks.

Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

@pg-hub-mirror pg-hub-mirror Bot locked and limited conversation to collaborators Sep 18, 2026
@pg-hub-mirror pg-hub-mirror Bot unlocked this conversation Sep 18, 2026
@pg-hub-mirror

pg-hub-mirror Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

Álvaro Herrera <alvherre(at)kurilemu(dot)de> via pgsql-hackers · original email

On 2026-Sep-16, Dmitry Dolgov wrote:

  • /* number of key attributes must match */
  • if (indexForm1->indnkeyatts != indexForm2->indnkeyatts)
  • return false;
    

I see that it was like this in the original commit, but isn't it too
restrictive regarding the goal stated in the function comment? If say
there are two unique indexes on columns (a), and (a, b), they have the
same understanding of what tuples will conflict, but the latter one will
not be used as an arbiter index. To be fair, I don't see how this may
become problem in practice, but still.
I'm not sure I understand this concern. Do you want to elaborate?
This functionality is there to support having two copies of "the same"
index during REINDEX CONCURRENTLY, and of course the second copy is
going to be identical in definition to the first one.
Maybe you want to propose a different name or a different comment for
this new function?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

@pg-hub-mirror pg-hub-mirror Bot locked and limited conversation to collaborators Sep 18, 2026
@pg-hub-mirror pg-hub-mirror Bot unlocked this conversation Sep 18, 2026
@pg-hub-mirror

pg-hub-mirror Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

Dmitry Dolgov <9erthalion6(at)gmail(dot)com> via pgsql-hackers · original email

On Fri, Sep 18, 2026 at 01:54:46PM +0200, Álvaro Herrera wrote:
On 2026-Sep-16, Dmitry Dolgov wrote:

  • /* number of key attributes must match */
  • if (indexForm1->indnkeyatts != indexForm2->indnkeyatts)
  •   return false;
    

I see that it was like this in the original commit, but isn't it too
restrictive regarding the goal stated in the function comment? If say
there are two unique indexes on columns (a), and (a, b), they have the
same understanding of what tuples will conflict, but the latter one will
not be used as an arbiter index. To be fair, I don't see how this may
become problem in practice, but still.

I'm not sure I understand this concern. Do you want to elaborate?

This functionality is there to support having two copies of "the same"
index during REINDEX CONCURRENTLY, and of course the second copy is
going to be identical in definition to the first one.

Maybe you want to propose a different name or a different comment for
this new function?
Yes, I get that. But the way how comment is written seems to emphasize
interchangeability of indexes in general, mentioning REINDEX
CONCURRENTLY as one use case, but not necessarily the only one -- or at
least it's my reading of it. Maybe a better commentary can solve it, but
since it's already pushed, consider it to be an optional nit pick.

@pg-hub-mirror pg-hub-mirror Bot locked and limited conversation to collaborators Sep 18, 2026
@pg-hub-mirror

pg-hub-mirror Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

Álvaro Herrera <alvherre(at)kurilemu(dot)de> via pgsql-hackers · original email

On 2026-Sep-18, Dmitry Dolgov wrote:

Yes, I get that. But the way how comment is written seems to emphasize
interchangeability of indexes in general, mentioning REINDEX
CONCURRENTLY as one use case, but not necessarily the only one -- or at
least it's my reading of it. Maybe a better commentary can solve it, but
since it's already pushed, consider it to be an optional nit pick.
I'll gladly take your nit pick.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
really, I see PHP as like a strange amalgamation of C, Perl, Shell
inflex: you know that "amalgam" means "mixture with mercury",
more or less, right?
i.e., "deadly poison"

@pg-hub-mirror pg-hub-mirror Bot unlocked this conversation Sep 18, 2026
@pg-hub-mirror pg-hub-mirror Bot locked and limited conversation to collaborators Sep 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area:sql SQL language or commands area:storage Storage, access methods, buffers, or I/O area:testing Tests and buildfarm source:pgsql-hackers Mirrored from pgsql-hackers type:patch Mail thread contains a PostgreSQL patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant