Обсуждение: [BUGS] BUG #14876: Segmentation fault with JSONB column used in store procthat gets used by view and later altered

The following bug has been logged on the website:

Bug reference:      14876
Logged by:          Samuel Horwitz
Email address:      samuel.horwitz@gmail.com
PostgreSQL version: 10.0
Operating system:   OS X (but running inside Docker official image)
Description:

I have a table that includes a JSONB column. I have a view that is based on
this table that reflects this JSONB column. I have yet another view, based
on the first view, that passes rows from the first view into a stored
PL/PGSQL procedure as anyelement. This stored procedure builds a JSON object
from it's input and returns that new JSON object. One of the values on the
new JSON object is the value of the JSONB column that has gone through two
views by this point, into the procedure.

At this point everything works. Selecting from the second view is fine.

However, if I alter the stored procedure's resulting JSON in any way, this
will cause selecting from the second view to trigger a segfault.

The solution is easy: regenerate the second view by just CREATE OR
REPLACEing it with the existing definition.

I have created a public Gist with reproduction steps and dumps, attached
here:
https://gist.github.com/samuelhorwitz/0bc77a517238914512fc8cdf50d217cd

Please scroll to the bottom (or click here
https://gist.github.com/samuelhorwitz/0bc77a517238914512fc8cdf50d217cd#gistcomment-2240442)
to see my steps.

This happens in 9.6.5 as well as 10.

Samuel Horwitz


--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Oct 26, 2017 at 11:40 AM,  <samuel.horwitz@gmail.com> wrote:
> I have created a public Gist with reproduction steps and dumps, attached
> here:
> https://gist.github.com/samuelhorwitz/0bc77a517238914512fc8cdf50d217cd
>
> Please scroll to the bottom (or click here
> https://gist.github.com/samuelhorwitz/0bc77a517238914512fc8cdf50d217cd#gistcomment-2240442)
> to see my steps.

I haven't looked at this thing in details. But this data may not be
present forever, say github.com is down or is removed from existence.
So if you can, please always attach any self-contained test case in a
way that it is saved in the archives of postgresql.org. In this case,
that would have been to email directly pgsql-bugs instead of using the
website form.

psql -f initial.sql
# This works
psql -c 'SELECT id, json FROM base_table_json'
psql -f break-it.sql
# this breaks
psql -c 'SELECT id, json FROM base_table_json'
psql -f fix-it.sql
# this works
psql -c 'SELECT id, json FROM base_table_json'

This reminds me of this case:
https://www.postgresql.org/message-id/20150707165212.1188.60819@wrigleys.postgresql.org.
The backtrace is not exactly the same, but that's really close.
-- 
Michael

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael Paquier <michael.paquier@gmail.com> writes:
> I haven't looked at this thing in details. But this data may not be
> present forever, say github.com is down or is removed from existence.
> So if you can, please always attach any self-contained test case in a
> way that it is saved in the archives of postgresql.org. In this case,
> that would have been to email directly pgsql-bugs instead of using the
> website form.

Thanks for posting the reproducer.  The attached seems to fix it, but
now that I've seen this, I wonder if there are other similar cases.

            regards, tom lane

diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index ca32a37..dae6fb9 100644
*** a/src/backend/parser/parse_relation.c
--- b/src/backend/parser/parse_relation.c
*************** expandRTE(RangeTblEntry *rte, int rtinde
*** 2207,2217 ****

                      if (colnames)
                      {
!                         /* Assume there is one alias per target item */
!                         char       *label = strVal(lfirst(aliasp_item));

                          *colnames = lappend(*colnames, makeString(pstrdup(label)));
-                         aliasp_item = lnext(aliasp_item);
                      }

                      if (colvars)
--- 2207,2235 ----

                      if (colnames)
                      {
!                         char       *label;
!
!                         /*
!                          * We prefer to use the outer query's column aliases
!                          * as column names.  However, in scenarios where
!                          * columns have been added to a view since the outer
!                          * query was originally parsed, there could be more
!                          * tlist items in the subquery than the outer query
!                          * knew about.  In such cases, use the subquery's
!                          * column names if it has any, else fall back to
!                          * "?column?".
!                          */
!                         if (aliasp_item)
!                         {
!                             label = strVal(lfirst(aliasp_item));
!                             aliasp_item = lnext(aliasp_item);
!                         }
!                         else if (te->resname)
!                             label = te->resname;
!                         else
!                             label = "?column?";

                          *colnames = lappend(*colnames, makeString(pstrdup(label)));
                      }

                      if (colvars)

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

I wrote:
> Thanks for posting the reproducer.  The attached seems to fix it, but
> now that I've seen this, I wonder if there are other similar cases.

After some not-very-exhaustive looking around, the only thing I've found
that seems closely related is expandRTE's behavior for a function
returning composite in an RTE_FUNCTION RTE.  It's clearly possible for
somebody to have added columns to the composite type since the calling
query was parsed, so there is a comparable hazard in that case as well.
But what that code path does is to ignore any columns beyond what it
saw originally (which it's memorialized in funccolcount; see the comment
for struct RangeTblFunction).

To be consistent with that, it seems like what the RTE_SUBQUERY case
ought to do is ignore columns beyond the length of eref->colnames.
This is probably less useful than what I posted first --- it means you
don't get to see any added columns in the result of "subqueryname.*".
But I doubt we want different behaviors in the two cases.

            regards, tom lane

diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index ca32a37..e89bebf 100644
*** a/src/backend/parser/parse_relation.c
--- b/src/backend/parser/parse_relation.c
*************** expandRTE(RangeTblEntry *rte, int rtinde
*** 2205,2213 ****
                      varattno++;
                      Assert(varattno == te->resno);

                      if (colnames)
                      {
-                         /* Assume there is one alias per target item */
                          char       *label = strVal(lfirst(aliasp_item));

                          *colnames = lappend(*colnames, makeString(pstrdup(label)));
--- 2205,2223 ----
                      varattno++;
                      Assert(varattno == te->resno);

+                     /*
+                      * In scenarios where columns have been added to a view
+                      * since the outer query was originally parsed, there can
+                      * be more items in the subquery tlist than the outer
+                      * query expects.  We should ignore such extra column(s)
+                      * --- compare the behavior for composite-returning
+                      * functions, in the RTE_FUNCTION case below.
+                      */
+                     if (!aliasp_item)
+                         break;
+
                      if (colnames)
                      {
                          char       *label = strVal(lfirst(aliasp_item));

                          *colnames = lappend(*colnames, makeString(pstrdup(label)));

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Oct 26, 2017 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> To be consistent with that, it seems like what the RTE_SUBQUERY case
> ought to do is ignore columns beyond the length of eref->colnames.
> This is probably less useful than what I posted first --- it means you
> don't get to see any added columns in the result of "subqueryname.*".
> But I doubt we want different behaviors in the two cases.

Sorry for coming up late in the game. I can see that you have pushed a
patch as d5b760e, but back-paddled a bit on d76886c. After some
analysis of things around, I think that you got it right. One comment
I have first though is that you could have used forboth as there is no
point to go through the target list entries once there are no more
aliases. Or target list entries marked as resjunk do not have an
expended reference name?
-- 
Michael


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael Paquier <michael.paquier@gmail.com> writes:
> Sorry for coming up late in the game. I can see that you have pushed a
> patch as d5b760e, but back-paddled a bit on d76886c. After some
> analysis of things around, I think that you got it right. One comment
> I have first though is that you could have used forboth as there is no
> point to go through the target list entries once there are no more
> aliases. Or target list entries marked as resjunk do not have an
> expended reference name?

Right, there's no entry in the outer RTE for resjunk columns.

(In practice, resjunk entries are at the end of the tlist so that it
wouldn't really matter, but I try to keep code from assuming that.)
        regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Fri, Oct 27, 2017 at 6:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> Sorry for coming up late in the game. I can see that you have pushed a
>> patch as d5b760e, but back-paddled a bit on d76886c. After some
>> analysis of things around, I think that you got it right. One comment
>> I have first though is that you could have used forboth as there is no
>> point to go through the target list entries once there are no more
>> aliases. Or target list entries marked as resjunk do not have an
>> expended reference name?
>
> Right, there's no entry in the outer RTE for resjunk columns.
>
> (In practice, resjunk entries are at the end of the tlist so that it
> wouldn't really matter, but I try to keep code from assuming that.)

OK, thanks for confirming. Yes the current logic is better this way.
-- 
Michael


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs