public inbox for [email protected]  

help / color / mirror / Atom feed

From: Alvaro Herrera <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Ranier Vilela <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Amit Langote <[email protected]>
Cc: Japin Li <[email protected]>
Cc: Zhihong Yu <[email protected]>
Cc: [email protected]
Cc: Tomas Vondra <[email protected]>
Cc: Daniel Westermann <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Jaime Casanova <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: support for MERGE
Date: Sat, 2 Apr 2022 17:02:01 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 2022-Mar-31, Daniel Gustafsson wrote:

> > On 31 Mar 2022, at 19:38, Ranier Vilela <[email protected]> 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/

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: support for MERGE
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox