It looks like we managed to access SqlInsert#isUpsert just by considering the original SqlNode while processing the plan https://github.com/diennea/herddb/pull/582 Having it in the Logical Plan would be better, but there is no need to complicate things by now.
In my case UPSERT is to be executed atomically with a row-level lock, so in my opinion it is different that a "MERGE INTO" that is more complicated. Julian: Having INSERT ... ON CONFLICT UPDATE would be a good option for me as well, but we should still support "UPSERT" keyword, at least in Babel, for compatibility reasons Thank you all Il giorno mer 18 mar 2020 alle ore 20:49 Julian Hyde <jh...@apache.org> ha scritto: > > The fact that PostgreSQL implements MERGE and “INSERT … ON CONFLICT” > (so-called UPSERT) differently with regard to consistency is an > implementation detail in PostgreSQL, and I refuse to accept it as rationale > for what we do in Calcite. (If we try to emulate the behavior of systems that > suck… well, it’s a race to mediocrity.) > > But I do accept that if we parse an UPSERT command, we should not throw away > the information in the SqlNode or RelNode representations. > > I think we should do what PostgreSQL has done[1]: add a ‘conflictBehavior’ > field, whose value is one of 3 enum values: ‘FAIL’ or ‘DO_NOTHING’ or > ‘UPDATE’, or something. So, the UPSERT command we implemented for Phoenix > would be "INSERT … ON CONFLICT UPDATE”. > > But we should not add complex actions like "ON CONFLICT (did) DO UPDATE SET > dname = EXCLUDED.dname” to INSERT. If people want complex actions, they > should use Calcite's MERGE command. If you want to translate a Calcite MERGE > to a PostgreSQL ‘INSERT … ON CONFLICT UPDATE …’, have at it. > > By the way, I don’t like the word ‘UPSERT’. It probably started an in-joke > among kernel developers. PostgreSQL doesn’t use it in their SQL (only in > their documentation), and neither should we. > > We had to add UPSERT to support Phoenix’s dialect (because Phoenix inherited > limitations on consistency from the underlying HBase engine) but we should > deprecate UPSERT and move to INSERT … ON CONFLICT instead. > > Julian > > [1] https://www.postgresql.org/docs/current/sql-insert.html > <https://www.postgresql.org/docs/current/sql-insert.html> > > > On Mar 18, 2020, at 2:07 AM, Enrico Olivelli <eolive...@gmail.com> wrote: > > > > Il Mer 18 Mar 2020, 08:35 Christian Beikov <christian.bei...@gmail.com > > <mailto:christian.bei...@gmail.com>> ha > > scritto: > > > >> I'd argue that these technical details are in fact the reason why this > >> is a different functionality, that deserves special handling. > >> > >> I expect an upsert statement like `INSERT INTO tbl(a, b) VALUES(1, 2) ON > >> CONFLICT DO NOTHING` to never produce a constraint violation. > >> > >> This is functionally different from a statement like `INSERT INTO tbl(a, > >> b) SELECT a, b (VALUES(1, 2)) x(a, b) WHERE NOT EXISTS(SELECT 1 FROM tbl > >> y WHERE x.a = y.a AND x.b = y.b)` which might cause the constraint > >> violation. > >> > >> On the other hand, an upsert statement like `INSERT INTO tbl(a, b) > >> VALUES(1, 2) ON CONFLICT DO UPDATE` guarantees that at the end of the > >> statement, there is the tuple `(1, 2)`. There are other variants that > >> provide different functionality(conflict handler conditions, special > >> update clause, etc.) and overall, for DBMS that do not support the ON > >> CONFLICT clause, it is necessary to fallback to PL/SQL to handle > >> constraint violations in exception handlers within loops to ensure the > >> same behvaior. > >> > >> If Calcite transforms such an UPSERT statement to a MERGE statement, it > >> must at least be flagged to require atomicity to be able to generate the > >> correct logic for the backend that this is running on. > >> > > > > Please don't do this. > > They are different features > > MERGE is more powerful and it is also for moving data from a table to > > another one (but I have never used MERGE) > > > > Enrico > > > > > >> Am 17.03.2020 um 22:28 schrieb Julian Hyde: > >>> I don't think there's a significant difference between the UPSERT and > >>> MERGE. The differences are in marketing (which keyword to use) and in > >>> technical details (e.g. concurrency semantics). Not worth splitting a > >>> core concept over. We spend a lot of effort keeping like-things-like. > >>> > >>> On Tue, Mar 17, 2020 at 1:11 AM Christian Beikov > >>> <christian.bei...@gmail.com> wrote: > >>>> AFAIK MERGE has different concurrency semantics than what some DBMS call > >>>> UPSERT. PostgreSQL for example has a guaranteed insert or update > >>>> semantics whereas MERGE could end with constraint violation errors: > >>>> https://wiki.postgresql.org/wiki/UPSERT > >>>> > >>>> Maybe it's worth adding that to the relational model after all? > >>>> > >>>> Am 17.03.2020 um 07:17 schrieb Enrico Olivelli: > >>>>> Il Lun 16 Mar 2020, 23:55 Julian Hyde <jhyde.apa...@gmail.com> ha > >> scritto: > >>>>> > >>>>>> Change the unparse operation for the dialect so that you generate > >> UPSERT > >>>>>> rather than MERGE. > >>>>>> > >>>>>> IIRC we did this for another dialect - maybe Phoenix. > >>>>>> > >>>>> Julian, > >>>>> In my case (HerdDB) I need to see the presence of UPSERT in the RelNode > >>>>> (EnumerableTableModify oŕ LogicalTableModify) > >>>>> I saw the JIRA where you introduced UPSERT for Phoenix > >>>>> I will check deeper, thanks > >>>>> > >>>>> Enrico > >>>>> > >>>>> > >>>>> > >>>>>> Julian > >>>>>> > >>>>>>> On Mar 16, 2020, at 1:22 AM, Enrico Olivelli <eolive...@gmail.com> > >>>>>> wrote: > >>>>>>> Il Lun 16 Mar 2020, 09:06 Stamatis Zampetakis <zabe...@gmail.com> > >> ha > >>>>>>> scritto: > >>>>>>> > >>>>>>>> Hi Enrico, > >>>>>>>> > >>>>>>>> I have the impression that UPSERT is currently supported only at the > >>>>>> parser > >>>>>>>> level [1] so it seems normal that you don't find something relevant > >> in > >>>>>>>> LogicalTableModify etc. Note that the SQL standard equivalent is the > >>>>>> MERGE > >>>>>>>> statement [2] but this also seems to be supported only at the > >>>>>>>> parser/validator level [2]. > >>>>>>>> I guess it is not necessary to introduce more things in TableModify > >>>>>> since > >>>>>>>> UPSERT seems to be syntactic sugar. I think that most of the work > >> can be > >>>>>>>> done in RelToSqlConverter [4] and possibly the rest of the code can > >> be > >>>>>> left > >>>>>>>> intact. > >>>>>>>> > >>>>>>> I would like to sens a patch that introduces the ability to keep the > >>>>>>> SqlInsert 'keywords' and pass them to TableModify? > >>>>>>> Would it be a good approach? > >>>>>>> > >>>>>>> The alternative is to introduce a new Operation but it would be a > >> more > >>>>>>> invasive change and I think it is not worth > >>>>>>> > >>>>>>> > >>>>>>> Enrico > >>>>>>> > >>>>>>> > >>>>>>>> Best, > >>>>>>>> Stamatis > >>>>>>>> > >>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-492 > >>>>>>>> [2] https://en.wikipedia.org/wiki/Merge_(SQL) > >>>>>>>> [3] https://issues.apache.org/jira/browse/CALCITE-985 > >>>>>>>> [4] > >>>>>>>> > >>>>>>>> > >>>>>> > >> https://github.com/apache/calcite/blob/d234626227954eefffe49f42abec65c649ffe3a6/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java#L2395 > >>>>>>>>> On Sun, Mar 15, 2020 at 6:53 PM Enrico Olivelli < > >> eolive...@gmail.com> > >>>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>> Hi, > >>>>>>>>> I am trying to use UPSERT but it seems to me that in TableModify > >> (or > >>>>>>>>> best LogicalTableModify or EnumerableTableModify) there is no way > >> to > >>>>>>>>> distinguish an INSERT from an UPSERT. > >>>>>>>>> > >>>>>>>>> Am I missing something? > >>>>>>>>> > >>>>>>>>> Regards > >>>>>>>>> Enrico >
