Horton

Thread

Commits

  1. adjust_partition_colnos mustn't be called if not needed

  2. Add support for MERGE SQL command

  1. support for MERGE

    Ranier Vilela <ranier.vf@gmail.com> — 2022-03-31T17:38:12Z

    On 2022-Mar-28, Alvaro Herrera wrote:

>> I intend to get this pushed after lunch.

>Pushed, with one more change: fetching the tuple ID junk attribute in
>ExecMerge was not necessary, since we already had done that in
>ExecModifyTable. We just needed to pass that down to ExecMerge, and
>make sure to handle the case where there isn't one.
Hi,

I think that there is an oversight at 7103ebb
<https://github.com/postgres/postgres/commit/7103ebb7aae8ab8076b7e85f335ceb8fe799097c>
There is no chance of Assert preventing this bug.

regards,
Ranier Vilela

  2. Re: support for MERGE

    Daniel Gustafsson <daniel@yesql.se> — 2022-03-31T18:10:35Z

    > On 31 Mar 2022, at 19:38, Ranier Vilela <ranier.vf@gmail.com> wrote:

> I think that there is an oversight at 7103ebb
> There is no chance of Assert preventing this bug.

This seems reasonable from brief reading of the code, NULL is a legitimate
value for the map and that should yield an empty list AFAICT.

--
Daniel Gustafsson		https://vmware.com/

  3. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-04-02T15:02:01Z

    On 2022-Mar-31, Daniel Gustafsson wrote:

> > On 31 Mar 2022, at 19:38, Ranier Vilela <ranier.vf@gmail.com> wrote:
> 
> > I think that there is an oversight at 7103ebb
> > There is no chance of Assert preventing this bug.
> 
> This seems reasonable from brief reading of the code, NULL is a legitimate
> value for the map and that should yield an empty list AFAICT.

There's no bug here and this is actually intentional: if the map is
NULL, this function should not be called.

In the code before this commit, there was an assert that this variable
was not null:

  static List *
  adjust_partition_colnos(List *colnos, ResultRelInfo *leaf_part_rri)
  {
-   List       *new_colnos = NIL;
    TupleConversionMap *map = ExecGetChildToRootMap(leaf_part_rri);
!   AttrMap    *attrMap;
    ListCell   *lc;
  
!   Assert(map != NULL);        /* else we shouldn't be here */
!   attrMap = map->attrMap;
  
    foreach(lc, colnos)
    {


We could add an Assert that map is not null in the new function, but
really there's no point: if the map is null, we'll crash just fine in
the following line.

I would argue that we should *remove* the Assert() that I left in
adjust_partition_colnos_with_map.

Even if we wanted to make the function handle the case of a NULL map,
then the right fix is not to return NIL, but rather we should return the
original list.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  4. Re: support for MERGE

    Andres Freund <andres@anarazel.de> — 2022-04-02T16:28:33Z

    Hi,

On 2022-04-02 17:02:01 +0200, Alvaro Herrera wrote:
> There's no bug here and this is actually intentional: if the map is
> NULL, this function should not be called.

This made me, again, wonder if we should add a pg_nonnull attibute to c.h. The
compiler can probably figure it out in this case, but there's plenty cases it
can't, because the function definition is in a different translation unit. And
IMO it helps humans too.

Regards,

Andres

  5. Re: support for MERGE

    Ranier Vilela <ranier.vf@gmail.com> — 2022-04-02T17:57:22Z

    Em sáb., 2 de abr. de 2022 às 12:01, Alvaro Herrera <alvherre@alvh.no-ip.org>
escreveu:

> On 2022-Mar-31, Daniel Gustafsson wrote:
>
> > > On 31 Mar 2022, at 19:38, Ranier Vilela <ranier.vf@gmail.com> wrote:
> >
> > > I think that there is an oversight at 7103ebb
> > > There is no chance of Assert preventing this bug.
> >
> > This seems reasonable from brief reading of the code, NULL is a
> legitimate
> > value for the map and that should yield an empty list AFAICT.
>
> There's no bug here and this is actually intentional: if the map is
> NULL, this function should not be called.
>
IMHO, actually there are bug here.
ExecGetChildToRootMap is clear, is possible returning NULL.
To discover if the map is NULL, ExecGetChildToRootMap needs to process
"ResultRelInfo *leaf_part_rri".
So, the argument "if the map is NULL, this function should not be called",
is contradictory.

Actually, with Assert at function adjust_partition_colnos_using_map,
will never be checked, because it crashed before, both
production and debug modes.


> In the code before this commit, there was an assert that this variable
> was not null:
>
>   static List *
>   adjust_partition_colnos(List *colnos, ResultRelInfo *leaf_part_rri)
>   {
> -   List       *new_colnos = NIL;
>     TupleConversionMap *map = ExecGetChildToRootMap(leaf_part_rri);
> !   AttrMap    *attrMap;
>     ListCell   *lc;
>
> !   Assert(map != NULL);        /* else we shouldn't be here */
> !   attrMap = map->attrMap;
>
>     foreach(lc, colnos)
>     {
>
>
> We could add an Assert that map is not null in the new function, but
> really there's no point: if the map is null, we'll crash just fine in
> the following line.
>
> I would argue that we should *remove* the Assert() that I left in
> adjust_partition_colnos_with_map.
>
> Even if we wanted to make the function handle the case of a NULL map,
> then the right fix is not to return NIL, but rather we should return the
> original list.
>
If the right fix is to return the original list, here is the patch attached.

regards
Ranier Vilela

  6. Re: support for MERGE

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-04-12T13:47:08Z

    On 2022-Apr-02, Ranier Vilela wrote:

> Em sáb., 2 de abr. de 2022 às 12:01, Alvaro Herrera <alvherre@alvh.no-ip.org>
> escreveu:

> IMHO, actually there are bug here.
> ExecGetChildToRootMap is clear, is possible returning NULL.
> To discover if the map is NULL, ExecGetChildToRootMap needs to process
> "ResultRelInfo *leaf_part_rri".
> So, the argument "if the map is NULL, this function should not be called",
> is contradictory.

I was not explicit enough.  I meant "if no map is needed to adjust
columns, then this function should not be called".  The caller already
knows if it's needed or not; it doesn't depend on literally testing
'map'.  If somebody mis-calls this function, it would have crashed, yes;
but that's a caller bug, not this function's.

A few days ago, the community Coverity also complained about this, so I
added an Assert that the map is not null, which should silence it.

> If the right fix is to return the original list, here is the patch attached.

... for a buggy caller (one that calls it when unnecessary), then yes
this would be the correct code -- except that now the caller doesn't
know if the returned list needs to be freed or not.  So it seems better
to avoid accumulating pointless calls to this function by just not
coping with them.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I suspect most samba developers are already technically insane...
Of course, since many of them are Australians, you can't tell." (L. Torvalds)

  7. Re: support for MERGE

    Ranier Vilela <ranier.vf@gmail.com> — 2022-04-12T14:19:00Z

    Em ter., 12 de abr. de 2022 às 10:47, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:

> On 2022-Apr-02, Ranier Vilela wrote:
>
> > Em sáb., 2 de abr. de 2022 às 12:01, Alvaro Herrera <
> alvherre@alvh.no-ip.org>
> > escreveu:
>
> > IMHO, actually there are bug here.
> > ExecGetChildToRootMap is clear, is possible returning NULL.
> > To discover if the map is NULL, ExecGetChildToRootMap needs to process
> > "ResultRelInfo *leaf_part_rri".
> > So, the argument "if the map is NULL, this function should not be
> called",
> > is contradictory.
>
> I was not explicit enough.  I meant "if no map is needed to adjust
> columns, then this function should not be called".  The caller already
> knows if it's needed or not; it doesn't depend on literally testing
> 'map'.  If somebody mis-calls this function, it would have crashed, yes;
> but that's a caller bug, not this function's.
>
Thanks for the explanation.


>
> A few days ago, the community Coverity also complained about this, so I
> added an Assert that the map is not null, which should silence it.
>
Thanks for hardening this.


>
> > If the right fix is to return the original list, here is the patch
> attached.
>
> ... for a buggy caller (one that calls it when unnecessary), then yes
> this would be the correct code -- except that now the caller doesn't
> know if the returned list needs to be freed or not.  So it seems better
> to avoid accumulating pointless calls to this function by just not
> coping with them.
>
 Sure, it is always better to avoid doing work, unless strictly necessary.

regards,
Ranier Vilela