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
