From: | Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Daniel Gustafsson <daniel(at)yesql(dot)se>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, Japin Li <japinli(at)hotmail(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>, simon(dot)riggs(at)enterprisedb(dot)co, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Daniel Westermann <dwe(at)dbi-services(dot)com>, Erik Rijkers <er(at)xs4all(dot)nl>, Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: support for MERGE |
Date: | 2022-04-02 17:57:22 |
Message-ID: | CAEudQApp3aHZ+Vfn0Drx1hc+=pOnvZ7dMxJqcbf5t_riS0ZXqA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Em sáb., 2 de abr. de 2022 às 12:01, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
escreveu:
> On 2022-Mar-31, Daniel Gustafsson wrote:
>
> > > On 31 Mar 2022, at 19:38, Ranier Vilela <ranier(dot)vf(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v2-0001-avoid-dereference-null-map.patch | application/octet-stream | 552 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Ranier Vilela | 2022-04-02 18:12:12 | logical decoding and replication of sequences |
Previous Message | Joe Conway | 2022-04-02 17:32:22 | Re: [PATCH v2] use has_privs_for_role for predefined roles |