Thread: BUG #14343: UPSERT (ON CONFLICT) doesn't check ON CONFLICT constraint first
BUG #14343: UPSERT (ON CONFLICT) doesn't check ON CONFLICT constraint first
From
reyes.r.ponce@gmail.com
Date:
VGhlIGZvbGxvd2luZyBidWcgaGFzIGJlZW4gbG9nZ2VkIG9uIHRoZSB3ZWJz aXRlOgoKQnVnIHJlZmVyZW5jZTogICAgICAxNDM0MwpMb2dnZWQgYnk6ICAg ICAgICAgIFJleWVzIFBvbmNlCkVtYWlsIGFkZHJlc3M6ICAgICAgcmV5ZXMu ci5wb25jZUBnbWFpbC5jb20KUG9zdGdyZVNRTCB2ZXJzaW9uOiA5LjUuMgpP cGVyYXRpbmcgc3lzdGVtOiAgIFJIRUwgNC44LjItMTYsIDY0LWJpdApEZXNj cmlwdGlvbjogICAgICAgIAoKLS0gQ3JlYXRlIGEgdGFibGUgd2l0aCBhIGtl eSwgb25lIG5vdCBudWxsIGNvbHVtbiwgYW5kIG9uZSBudWxsYWJsZQpjb2x1 bW4uDQoNCkNSRUFURSBUQUJMRSBwdWJsaWMuTXlUYWJsZQ0KKA0KCU1ZX0lE IGludGVnZXIgcHJpbWFyeSBrZXkgTk9UIE5VTEwsDQoJQ09MMSAgaW50ZWdl ciAgIE5PVCBOVUxMLA0KCUNPTDIgIGludGVnZXIsDQoJQ1JFVE5fVFMgICAg ICB0aW1lc3RhbXAgd2l0aCB0aW1lIHpvbmUgICBOT1QgTlVMTCwNCglDUkVU Tl9VU0VSX0lEIHZhcmNoYXIgICBOT1QgTlVMTCwNCglVUERUX1RTICAgICAg IHRpbWVzdGFtcCB3aXRoIHRpbWUgem9uZSAgLA0KCVVQRFRfVVNFUl9JRCAg dmFyY2hhciAgDQopOw0KDQotLSBDcmVhdGUgYSBmdW5jdGlvbiB0byB1cHNl cnQgd2hhdGV2ZXIgaXMgcGFzc2VkIGluLiANCg0KQ1JFQVRFIE9SIFJFUExB Q0UgRlVOQ1RJT04gcHVibGljLnVwc2VydF9teXRhYmxlKA0KICAgIGlfbXlf aWQgaW50ZWdlciwNCiAgICBpX2NvbDF2YWwgaW50ZWdlciBERUZBVUxUIE5V TEw6OmludGVnZXIsDQogICAgaV9jb2wydmFsIGludGVnZXIgREVGQVVMVCBO VUxMOjppbnRlZ2VyDQogICAgKQ0KICBSRVRVUk5TIGpzb24gQVMNCiRCT0RZ JA0KREVDTEFSRSANCiAgICBnZXRfcm93cyBqc29uOw0KICAgIGJlZ2luU3Fs IHRleHQgOj0gDQogICAgJ0lOU0VSVCBJTlRPIHB1YmxpYy5NeVRhYmxlKA0K ICAgICAgICAgICAgTVlfSUQsIENPTDEsIENPTDIsIENSRVROX1RTLCBDUkVU Tl9VU0VSX0lELCBVUERUX1RTLApVUERUX1VTRVJfSUQpDQogICAgVkFMVUVT ICgkMSwgJDIsICQzLCBOT1coKSwgY3VycmVudF91c2VyLCBOT1coKSwgY3Vy cmVudF91c2VyKSANCiAgICBPTiBDT05GTElDVChNWV9JRCkgDQogICAgRE8g VVBEQVRFIFNFVCBVUERUX1RTID0gTk9XKCksIFVQRFRfVVNFUl9JRCA9IGN1 cnJlbnRfdXNlciwgJzsNCiAgICBjb21tYSB0ZXh0IDo9ICcnOw0KICAgIGVu ZFNxbCB0ZXh0IDo9ICcnOw0KICAgIHNxbCB0ZXh0IDo9ICcnOw0KQkVHSU4N Cg0KLS0gVmFsaWRhdGUgaW5wdXQgcGFyYW1ldGVycyAtIE1VU1QgU1BFQ0lG WSBQSw0KDQpJRiAoIGlfbXlfaWQgaXMgTlVMTCBPUiBpX215X2lkID0gJzAn KSBUSEVODQogICAgUmFpc2UgRXhjZXB0aW9uICdJTlZBTElEIElOUFVUOiBJ ZCBjYW5ub3QgYmUgbnVsbCcgdXNpbmcgZXJyY29kZSA9CiczJzsNCkVORCBJ RjsNCg0KLyogQ3JlYXRlIG1pZGRsZSBTUUwgKHRoZSBmaWVsZHMgdG8gdXBk YXRlKS4NCiANCiAgIEFmdGVyIHdlIHNlZSB0aGUgZmlyc3Qgbm9uLW51bGwg cGFyYW1ldGVyLCBzZXQgdGhlIGNvbW1hIHRvIGEgY29tbWEKaW5zdGVhZCBv ZiBibGFuay4uLiAgDQogICANCiAgIFRoZSAkIHZhcmlhYmxlcyBsaW5lIHVw IHdpdGggdGhlIG9yZGVyIGluIHRoZSAiRVhFQ1VURSBzcWwgVVNJTkciCnN0 YXRlbWVudCBiZWxvdywgbm90IGluIG9yZGVyIGluIHRoZSBwYXJhbWV0ZXIg bGlzdCBhYm92ZS4gICAgIA0KICAgKi8NCklGIChpX2NvbDF2YWwgaXMgbm90 IE5VTEwpIFRIRU4NCiAgICBlbmRTcWwgOj0gZW5kU3FsIHx8IGNvbW1hIHx8 ICdDT0wxID0gJDInOw0KICAgIGNvbW1hIDo9ICcsICc7ICAgICAgIA0KRU5E IElGOw0KDQpJRiAoaV9jb2wydmFsIGlzIG5vdCBOVUxMKSBUSEVODQogICAg ZW5kU3FsIDo9IGVuZFNxbCB8fCBjb21tYSB8fCAnQ09MMiA9ICQzJzsNCiAg ICBjb21tYSA6PSAnLCAnOyAgICAgICANCkVORCBJRjsNCg0KLS0gcmFpc2Ug Tk9USUNFICclJywgYmVnaW5TcWw7ICAvKiBERUJVR0dJTkcgQ09ERSAgLSBP dXRwdXRzIG1lc3NhZ2UgaW4KcGxBZG1pbiBtZXNzYWdlcyB0YWIuICovDQot LSByYWlzZSBOT1RJQ0UgJyUnLCBlbmRTcWw7ICAgIC8qIERFQlVHR0lORyBD T0RFICAtIE91dHB1dHMgbWVzc2FnZSBpbgpwbEFkbWluIG1lc3NhZ2VzIHRh Yi4gKi8NCg0Kc3FsIDo9IGJlZ2luU3FsIHx8IGVuZFNxbDsgDQoNCi0tIHJh aXNlIE5PVElDRSAnJScsIHNxbDsgIC8qIERFQlVHR0lORyBDT0RFIC0gT3V0 cHV0cyBtZXNzYWdlIGluIHBsQWRtaW4KbWVzc2FnZXMgdGFiLiAqLw0KDQov KiBUaGUgJCB2YXJpYWJsZXMgbGluZSB1cCB3aXRoIHRoZSBvcmRlciBpbiB0 aGUgVVNJTkcgY2xhdXNlIGJlbG93LCBub3QgaW4Kb3JkZXIgaW4gdGhlIHBh cmFtZXRlciBsaXN0LiAgKi8NCkVYRUNVVEUgc3FsDQogICBVU0lORyBpX215 X2lkLCBpX2NvbDF2YWwsIGlfY29sMnZhbDsgDQoNCi0tIFJldHVybiBqc29u Lg0KDQpzZWxlY3Qgcm93X3RvX2pzb24oZikgSU5UTyBnZXRfcm93cw0KICBm cm9tICgNCiAgICAgU0VMRUNUIE1ZX0lELCBDT0wxLCBDT0wyLCBjcmV0bl90 cywgY3JldG5fdXNlcl9pZCwgdXBkdF90cywKdXBkdF91c2VyX2lkDQogICAg IEZST00gcHVibGljLk15VGFibGUNCiAgICAgV0hFUkUgTVlfSUQgPSBpX215 X2lkKSBmOw0KDQpSRVRVUk4gZ2V0X3Jvd3M7DQoNCkVORA0KJEJPRFkkDQog IExBTkdVQUdFIHBscGdzcWwgVk9MQVRJTEUNCiAgQ09TVCAxMDA7DQoNCg0K LS0gQ2FsbCB0aGUgU1AgZm9yIGluaXRpYWwgaW5zZXJ0IChhbGwgY29sdW1u cykuIFdvcmtzIGZpbmUuDQpTRUxFQ1QgcHVibGljLnVwc2VydF9teXRhYmxl KA0KICAgIDEsDQogICAgMiwNCiAgICAzDQopOw0KDQoNCi0tIENhbGwgdGhl IFNQIGZvciB1cGRhdGUgb2YgdGhlIG51bGxhYmxlIGNvbHVtbi4gRXJyb3Ig YmVjYXVzZSBub3QgY2hlY2tpbmcKT04gQ09ORkxJQ1QgY29uZGl0aW9uIGZp cnN0Lg0KU0VMRUNUIHB1YmxpYy51cHNlcnRfbXl0YWJsZSgNCiAgICAxLA0K ICAgIE5VTEwsDQogICAgNQ0KKTsNCg0KDQovKg0KRVJST1I6ICBudWxsIHZh bHVlIGluIGNvbHVtbiAiY29sMSIgdmlvbGF0ZXMgbm90LW51bGwgY29uc3Ry YWludA0KREVUQUlMOiAgRmFpbGluZyByb3cgY29udGFpbnMgKDEsIG51bGws IDUsIDIwMTYtMDktMjcgMTc6MzI6NTEuMDU0ODk2KzAwLApwbF9tc3RyX3Vz ciwgMjAxNi0wOS0yNyAxNzozMjo1MS4wNTQ4OTYrMDAsIHBsX21zdHJfdXNy KS4NCkNPTlRFWFQ6ICBTUUwgc3RhdGVtZW50ICJJTlNFUlQgSU5UTyBwdWJs aWMuTXlUYWJsZSgNCiAgICAgICAgICAgIE1ZX0lELCBDT0wxLCBDT0wyLCBD UkVUTl9UUywgQ1JFVE5fVVNFUl9JRCwgVVBEVF9UUywKVVBEVF9VU0VSX0lE KQ0KICAgIFZBTFVFUyAoJDEsICQyLCAkMywgTk9XKCksIGN1cnJlbnRfdXNl ciwgTk9XKCksIGN1cnJlbnRfdXNlcikgDQogICAgT04gQ09ORkxJQ1QoTVlf SUQpIA0KICAgIERPIFVQREFURSBTRVQgVVBEVF9UUyA9IE5PVygpLCBVUERU X1VTRVJfSUQgPSBjdXJyZW50X3VzZXIsIENPTDIgPSAkMyINClBML3BnU1FM IGZ1bmN0aW9uIHVwc2VydF9teXRhYmxlKGludGVnZXIsaW50ZWdlcixpbnRl Z2VyKSBsaW5lIDQ2IGF0CkVYRUNVVEUNCg0KKioqKioqKioqKiBFcnJvciAq KioqKioqKioqDQoNCkVSUk9SOiBudWxsIHZhbHVlIGluIGNvbHVtbiAiY29s MSIgdmlvbGF0ZXMgbm90LW51bGwgY29uc3RyYWludA0KU1FMIHN0YXRlOiAy MzUwMg0KRGV0YWlsOiBGYWlsaW5nIHJvdyBjb250YWlucyAoMSwgbnVsbCwg NSwgMjAxNi0wOS0yNyAxNzozMjo1MS4wNTQ4OTYrMDAsCnBsX21zdHJfdXNy LCAyMDE2LTA5LTI3IDE3OjMyOjUxLjA1NDg5NiswMCwgcGxfbXN0cl91c3Ip Lg0KQ29udGV4dDogU1FMIHN0YXRlbWVudCAiSU5TRVJUIElOVE8gcHVibGlj Lk15VGFibGUoDQogICAgICAgICAgICBNWV9JRCwgQ09MMSwgQ09MMiwgQ1JF VE5fVFMsIENSRVROX1VTRVJfSUQsIFVQRFRfVFMsClVQRFRfVVNFUl9JRCkN CiAgICBWQUxVRVMgKCQxLCAkMiwgJDMsIE5PVygpLCBjdXJyZW50X3VzZXIs IE5PVygpLCBjdXJyZW50X3VzZXIpIA0KICAgIE9OIENPTkZMSUNUKE1ZX0lE KSANCiAgICBETyBVUERBVEUgU0VUIFVQRFRfVFMgPSBOT1coKSwgVVBEVF9V U0VSX0lEID0gY3VycmVudF91c2VyLCBDT0wyID0gJDMiDQpQTC9wZ1NRTCBm dW5jdGlvbiB1cHNlcnRfbXl0YWJsZShpbnRlZ2VyLGludGVnZXIsaW50ZWdl cikgbGluZSA0NiBhdApFWEVDVVRFDQoNCi0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tDQoNCk9OIENPTkZMSUNUIHNob3VsZCBoYXZlIGJlZW4gY2hlY2tl ZCBiZWZvcmUgdGhlIGF0dGVtcHQgdG8gaW5zZXJ0IQ0KSWYgaSBpbXBsZW1l bnQgYXMgQ1RFIGluc3RlYWQgb2YgdXNpbmcgdGhlIG5ldyBzeW50YXggaXQg d29ya3MgZmluZS4NCk5vdCBjaGVja2luZyBPTiBDT05GTElDVCBjb25kaXRp b24gZmlyc3QgbWFrZXMgdGhlIG5ldyBzeW50YXggYXBwbGljYWJsZQp0bw0K ZmV3ZXIgc2NlbmFyaW9zIHRoYW4gaXQgc2hvdWxkIGJlIGFibGUgdG8gYmUg dXNlZCBmb3IuIEl0J3MgdXNlbGVzcyBmb3IgdGhpcwpwYXJ0aWN1bGFyIHVz ZSBjYXNlLi4uIA0KDQoqLw0KCgo=
reyes.r.ponce@gmail.com writes:
> ERROR: null value in column "col1" violates not-null constraint
> DETAIL: Failing row contains (1, null, 5, 2016-09-27 17:32:51.054896+00,
> pl_mstr_usr, 2016-09-27 17:32:51.054896+00, pl_mstr_usr).
> CONTEXT: SQL statement "INSERT INTO public.MyTable(
> MY_ID, COL1, COL2, CRETN_TS, CRETN_USER_ID, UPDT_TS,
> UPDT_USER_ID)
> VALUES ($1, $2, $3, NOW(), current_user, NOW(), current_user)
> ON CONFLICT(MY_ID)
> DO UPDATE SET UPDT_TS = NOW(), UPDT_USER_ID = current_user, COL2 = $3"
> PL/pgSQL function upsert_mytable(integer,integer,integer) line 46 at
> EXECUTE
This test case seems rather overcomplicated, but AFAICS you are
complaining because the NOT NULL constraint is checked before uniqueness
is checked. Sorry, that is not a bug, that is by design.
regards, tom lane
Re: BUG #14343: UPSERT (ON CONFLICT) doesn't check ON CONFLICT constraint first
From
Reyes Ponce
Date:
Hi Tom, That may be "the design" and it may be a fine design for insert, but it's not a great design for upsert, which was sort of my point. An update statement does not require all NOT NULL columns to be specified so neither should an upsert require all NOT NULL columns to be specified in the case where the update will run. Any chance you guys will do a UPDATE ... ON MISSING... DO INSERT... version as I expect in that case it would be implemented closer to the functionality you get implementing upsert with a CTE (and how upsert in most NoSql DB works (i.e. doesn't impose more restrictions than update in the update case)) which would cover far more use cases than the current design of INSERT... ON CONFLICT... DO UPDATE...? Thanks for all the hard work. I've only been using Postgres for a few months, but thus far it's been solid. -- Reyes On 9/27/2016 3:06 PM, Tom Lane wrote: > reyes.r.ponce@gmail.com writes: >> ERROR: null value in column "col1" violates not-null constraint >> DETAIL: Failing row contains (1, null, 5, 2016-09-27 17:32:51.054896+00, >> pl_mstr_usr, 2016-09-27 17:32:51.054896+00, pl_mstr_usr). >> CONTEXT: SQL statement "INSERT INTO public.MyTable( >> MY_ID, COL1, COL2, CRETN_TS, CRETN_USER_ID, UPDT_TS, >> UPDT_USER_ID) >> VALUES ($1, $2, $3, NOW(), current_user, NOW(), current_user) >> ON CONFLICT(MY_ID) >> DO UPDATE SET UPDT_TS = NOW(), UPDT_USER_ID = current_user, COL2 = $3" >> PL/pgSQL function upsert_mytable(integer,integer,integer) line 46 at >> EXECUTE > This test case seems rather overcomplicated, but AFAICS you are > complaining because the NOT NULL constraint is checked before uniqueness > is checked. Sorry, that is not a bug, that is by design. > > regards, tom lane >
Re: BUG #14343: UPSERT (ON CONFLICT) doesn't check ON CONFLICT constraint first
From
Peter Geoghegan
Date:
On Fri, Sep 30, 2016 at 8:19 PM, Reyes Ponce <reyes.r.ponce@gmail.com> wrote: > Any chance you guys will do a > > UPDATE ... ON MISSING... DO INSERT... > > version as I expect in that case it would be implemented closer to the > functionality you get implementing upsert with a CTE (and how upsert in > most NoSql DB works (i.e. doesn't impose more restrictions than update > in the update case)) which would cover far more use cases than the > current design of INSERT... ON CONFLICT... DO UPDATE...? I don't think that that makes sense. If you know ahead of time that the INSERT path will definitely throw an error, you're either relying on that path never being taken, in which case you should just use a plain UPDATE, or you have a bug in your application code, in which case you should be glad to have it surfaced sooner rather than later. -- Peter Geoghegan
Re: BUG #14343: UPSERT (ON CONFLICT) doesn't check ON CONFLICT constraint first
From
Reyes Ponce
Date:
Hi Peter, In my case I'm writing a stored procedure that will insert or update based on whether the row already exists. Some things calling it will be interactive and some will be batch processes. Sometimes all the parameters will be not null and sometimes some of the parameters will be null. The stored procedure should do the right thing (insert or update) as best it can or throw an error. Regardless, of this specific design, the following are still true: 1) Upsert is becoming a common feature (probably because it matches well with the definition of REST PUT and POST functionality). 2) The current INSERT... ON CONFLICT... DO UPDATE... doesn't handle the scenarios stated in my initial email, which upsert functionality can usually handle. 3) Upsert can be done with CTEs which can handle those scenarios but has potential race conditions. which is why I am asking about this (i.e. if the insert version of upsert can't handle these scenarios, maybe we need an upsert based on update). On Oct 1, 2016 8:21 AM, "Peter Geoghegan" <pg@heroku.com> wrote: > On Fri, Sep 30, 2016 at 8:19 PM, Reyes Ponce <reyes.r.ponce@gmail.com> > wrote: > > Any chance you guys will do a > > > > UPDATE ... ON MISSING... DO INSERT... > > > > version as I expect in that case it would be implemented closer to the > > functionality you get implementing upsert with a CTE (and how upsert in > > most NoSql DB works (i.e. doesn't impose more restrictions than update > > in the update case)) which would cover far more use cases than the > > current design of INSERT... ON CONFLICT... DO UPDATE...? > > I don't think that that makes sense. If you know ahead of time that > the INSERT path will definitely throw an error, you're either relying > on that path never being taken, in which case you should just use a > plain UPDATE, or you have a bug in your application code, in which > case you should be glad to have it surfaced sooner rather than later. > > -- > Peter Geoghegan >
Re: BUG #14343: UPSERT (ON CONFLICT) doesn't check ON CONFLICT constraint first
From
Peter Geoghegan
Date:
On Wed, Oct 5, 2016 at 7:34 PM, Reyes Ponce <reyes.r.ponce@gmail.com> wrote: > 1) Upsert is becoming a common feature (probably because it matches well > with the definition of REST PUT and POST functionality). > > 2) The current INSERT... ON CONFLICT... DO UPDATE... doesn't handle the > scenarios stated in my initial email, which upsert functionality can usually > handle. > > 3) Upsert can be done with CTEs which can handle those scenarios but has > potential race conditions. I think that your stored procedure needs to learn about the different cases. Simple as that. If an INSERT would fail, you have no right to assume an upsert that *might* take the insert path, but doesn't this time, should not fail all the time. It's not as if a NOT NULL constraint is based on anything other than the simple fact that the row that you mean to INSERT has attributes that are NULL but shouldn't be. Unlike a unique constraint, it doesn't matter what anybody else may be doing at the same time, or may have inserted even before you began -- your tuple is definitely going to violate the constraint. > which is why I am asking about this (i.e. if the insert version of upsert > can't handle these scenarios, maybe we need an upsert based on update). For reasons that are rather complicated, "an upsert based on update" is more or less an oxymoron. Basically, all of the useful upsert guarantees hinge upon the implementation taking the alternative path in the event of a would-be duplicate violation specifically. You can't make that work with UPDATE, because it would have to be based on something *not* existing, which is an impossibly ticklish condition to rely on, unless you lock the entire table or something. -- Peter Geoghegan