support for MERGE

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/

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/

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

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

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)

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