Обсуждение: pg_dump bug for extension owned tables

Found when working against release 12.


Given the following extension:


::::::::::::::
share/postgresql/extension/dummy--1.0.0.sql
::::::::::::::
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION dummy" to load this file. \quit

CREATE TABLE @extschema@.dummytab (
       a int,
       b int,
       c int);
SELECT pg_catalog.pg_extension_config_dump('dummytab', '');


::::::::::::::
share/postgresql/extension/dummy.control
::::::::::::::
# dummy extension
comment = 'dummy'
default_version = '1.0.0'
relocatable = false


and this use of it:


bin/psql -c 'create schema dummy; create extension  dummy schema dummy;
insert into dummy.dummytab values(1,2,3);'


this command segfaults:


bin/pg_dump -a --column-inserts -n dummy


It appears that for extension owned tables tbinfo.attgenerated isn't
being properly populated, so line 2050 in REL_12_STABLE, which is line
2109 in git tip, is failing.


I'm looking for a fix, but if anyone has a quick fix that would be nice :-)


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 6/26/20 9:57 AM, Andrew Dunstan wrote:
> It appears that for extension owned tables tbinfo.attgenerated isn't
> being properly populated, so line 2050 in REL_12_STABLE, which is line
> 2109 in git tip, is failing.
>
>

Should have mentioned this is in src/bin/pg_dump/pg_dump.c


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 6/26/20 2:10 PM, Fabrízio de Royes Mello wrote:
>
> On Fri, Jun 26, 2020 at 11:55 AM Fabrízio de Royes Mello
> <fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>> wrote:
> >
> >
> > On Fri, Jun 26, 2020 at 11:24 AM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com
> <mailto:andrew.dunstan@2ndquadrant.com>> wrote:
> > >
> > >
> > > On 6/26/20 9:57 AM, Andrew Dunstan wrote:
> > > > It appears that for extension owned tables tbinfo.attgenerated isn't
> > > > being properly populated, so line 2050 in REL_12_STABLE, which
> is line
> > > > 2109 in git tip, is failing.
> > > >
> > > >
> > >
> > > Should have mentioned this is in src/bin/pg_dump/pg_dump.c
> > >
> >
> > Having a look on it.
> >
>
> Seems when qualify the schemaname the the "tbinfo->interesting" field
> is not setted for extensions objects, so the getTableAttrs can't fill
> the attgenerated field properly.
>
> I'm not 100% sure it's the correct way but the attached patch works
> for me and all tests passed. Maybe we should add more TAP tests?
>
>


Thanks for this.

It looks sane to me, I'll let others weigh in on it though. Yes we
should have a TAP test for it.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 6/26/20 2:10 PM, Fabrízio de Royes Mello wrote:
>
> On Fri, Jun 26, 2020 at 11:55 AM Fabrízio de Royes Mello
> <fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>> wrote:
> >
> >
> > On Fri, Jun 26, 2020 at 11:24 AM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com
> <mailto:andrew.dunstan@2ndquadrant.com>> wrote:
> > >
> > >
> > > On 6/26/20 9:57 AM, Andrew Dunstan wrote:
> > > > It appears that for extension owned tables tbinfo.attgenerated isn't
> > > > being properly populated, so line 2050 in REL_12_STABLE, which
> is line
> > > > 2109 in git tip, is failing.
> > > >
> > > >
> > >
> > > Should have mentioned this is in src/bin/pg_dump/pg_dump.c
> > >
> >
> > Having a look on it.
> >
>
> Seems when qualify the schemaname the the "tbinfo->interesting" field
> is not setted for extensions objects, so the getTableAttrs can't fill
> the attgenerated field properly.
>
> I'm not 100% sure it's the correct way but the attached patch works
> for me and all tests passed. Maybe we should add more TAP tests?
>
>


I just tried this patch out on master, with the test case I gave
upthread. It's not working, still getting a segfault.


Will look closer.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 7/13/20 10:52 AM, Fabrízio de Royes Mello wrote:
>
> On Sat, Jul 11, 2020 at 8:07 PM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com
> <mailto:andrew.dunstan@2ndquadrant.com>> wrote:
> >
> >
> > On 6/26/20 2:10 PM, Fabrízio de Royes Mello wrote:
> > >
> > > On Fri, Jun 26, 2020 at 11:55 AM Fabrízio de Royes Mello
> > > <fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>
> <mailto:fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>>> wrote:
> > > >
> > > >
> > > > On Fri, Jun 26, 2020 at 11:24 AM Andrew Dunstan
> > > <andrew.dunstan@2ndquadrant.com
> <mailto:andrew.dunstan@2ndquadrant.com>
> > > <mailto:andrew.dunstan@2ndquadrant.com
> <mailto:andrew.dunstan@2ndquadrant.com>>> wrote:
> > > > >
> > > > >
> > > > > On 6/26/20 9:57 AM, Andrew Dunstan wrote:
> > > > > > It appears that for extension owned tables
> tbinfo.attgenerated isn't
> > > > > > being properly populated, so line 2050 in REL_12_STABLE, which
> > > is line
> > > > > > 2109 in git tip, is failing.
> > > > > >
> > > > > >
> > > > >
> > > > > Should have mentioned this is in src/bin/pg_dump/pg_dump.c
> > > > >
> > > >
> > > > Having a look on it.
> > > >
> > >
> > > Seems when qualify the schemaname the the "tbinfo->interesting" field
> > > is not setted for extensions objects, so the getTableAttrs can't fill
> > > the attgenerated field properly.
> > >
> > > I'm not 100% sure it's the correct way but the attached patch works
> > > for me and all tests passed. Maybe we should add more TAP tests?
> > >
> > >
> >
> >
> > I just tried this patch out on master, with the test case I gave
> > upthread. It's not working, still getting a segfault.
> >
>
> Ohh really sorry about it... my bad... i completely forgot about it!!!
>
> Due to my rush I ended up adding the wrong patch version. Attached the
> correct version.
>
> Will add TAP tests to src/test/modules/test_pg_dump


yeah, that's the fix I came up with too. The only thing I added was
"Assert(tbinfo->attgenerated);" at about line 2097.


Will wait for your TAP tests.


thanks


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 7/13/20 2:46 PM, Fabrízio de Royes Mello wrote:
>
>
> >
> > yeah, that's the fix I came up with too. The only thing I added was
> > "Assert(tbinfo->attgenerated);" at about line 2097.
> >
>
> Cool.
>
> >
> > Will wait for your TAP tests.
> >
>
> Actually I've sent it already but it seems to have gone to the
> moderation queue.
>
> Anyway attached with your assertion and TAP tests.
>
>



Thanks, that all seems fine. The TAP test changes are a bit of a pain in
the neck before release 11, so I think I'll just do those back that far,
but the main fix for all live branches.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Aug 6, 2020 at 4:12 PM Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
>
> On Mon, Jul 13, 2020 at 6:18 PM Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>>
>>
>> On Mon, Jul 13, 2020 at 5:05 PM Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:
>> >
>> >
>> > On 7/13/20 2:46 PM, Fabrízio de Royes Mello wrote:
>> > >
>> > >
>> > > >
>> > > > yeah, that's the fix I came up with too. The only thing I added was
>> > > > "Assert(tbinfo->attgenerated);" at about line 2097.
>> > > >
>> > >
>> > > Cool.
>> > >
>> > > >
>> > > > Will wait for your TAP tests.
>> > > >
>> > >
>> > > Actually I've sent it already but it seems to have gone to the
>> > > moderation queue.
>> > >
>> > > Anyway attached with your assertion and TAP tests.
>> > >
>> > >
>> >
>> >
>> >
>> > Thanks, that all seems fine. The TAP test changes are a bit of a pain in
>> > the neck before release 11, so I think I'll just do those back that far,
>> > but the main fix for all live branches.
>> >
>>
>> Sounds good to me.
>>
>
> Just added to the next commitfest [1] to make sure we'll not lose it.
>
> Regards,
>
> [1] https://commitfest.postgresql.org/29/2671/
>


Thanks, Committed. Further investigation shows this was introduced in
release 12, so that's how far back I went.

cheers

andrew


--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> Thanks, Committed. Further investigation shows this was introduced in
> release 12, so that's how far back I went.

Still further investigation shows that this patch caused bug #16655 [1].
It should *not* have been designed to unconditionally clear the
table's "interesting" flag, as there may have been other reasons
why that was set.  The right way to think about it is "if we are
going to dump the table's data, then the table certainly needs its
interesting flag set, so that we'll collect the per-attribute info.
Otherwise leave well enough alone".

The patches I proposed in the other thread seem like they really ought
to go all the way back for safety's sake.  However, I do not observe
any crash on the test case in v11, and I'm kind of wondering why not.
Did you identify exactly where this was "introduced in release 12"?

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/16655-5c92d6b3a9438137%40postgresql.org

On 10/6/20 5:19 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> Thanks, Committed. Further investigation shows this was introduced in
>> release 12, so that's how far back I went.
> Still further investigation shows that this patch caused bug #16655 [1].
> It should *not* have been designed to unconditionally clear the
> table's "interesting" flag, as there may have been other reasons
> why that was set.  The right way to think about it is "if we are
> going to dump the table's data, then the table certainly needs its
> interesting flag set, so that we'll collect the per-attribute info.
> Otherwise leave well enough alone".



Yes, I see the issue. Mea culpa :-(



>
> The patches I proposed in the other thread seem like they really ought
> to go all the way back for safety's sake.  However, I do not observe
> any crash on the test case in v11, and I'm kind of wondering why not.
> Did you identify exactly where this was "introduced in release 12"?



It looks like you've since discovered the cause here. Do you need me to
dig more?


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> It looks like you've since discovered the cause here. Do you need me to
> dig more?

Nah, I've got it.  Thanks.

            regards, tom lane