Date: Sat, 2 Apr 2022 17:02:01 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Ranier Vilela <ranier.vf@gmail.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Amit Langote <amitlangote09@gmail.com>,
	Japin Li <japinli@hotmail.com>, Zhihong Yu <zyu@yugabyte.com>,
	simon.riggs@enterprisedb.co,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Daniel Westermann <dwe@dbi-services.com>,
	Erik Rijkers <er@xs4all.nl>,
	Jaime Casanova <jcasanov@systemguards.com.ec>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: support for MERGE
Message-ID: <202204021502.hxkog726fulu@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3D47D785-0788-43BE-93B4-06B13B8722FA@yesql.se>
Archived-At: 
 <https://www.postgresql.org/message-id/202204021502.hxkog726fulu%40alvherre.pgsql>
Precedence: bulk

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/