Horton

Thread

Commits

  1. Remove bogus Assert in foreign key cloning code

  1. create partition table caused server crashed with self-referencing foreign key

    Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> — 2020-04-22T07:50:55Z

    Hi,

Getting a server crash while creating partition table which  have
self-referencing foreign key

postgres=# CREATE TABLE part1 (c1 int PRIMARY KEY, c2 int REFERENCES part1)
PARTITION BY LIST (c1);
CREATE TABLE
postgres=# CREATE TABLE part1_p1 PARTITION OF part1 FOR VALUES IN (1);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

--stack-trace
[edb@localhost bin]$ gdb -q -c data/core.16883 postgres
Core was generated by `postgres: edb postgres [local] CREATE TABLE
    '.
Program terminated with signal 6, Aborted.
#0  0x00000039212324f5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
64  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-24.el6.x86_64 libgcc-4.4.7-23.el6.x86_64
libselinux-2.0.94-7.el6.x86_64 openssl-1.0.1e-58.el6_10.x86_64
zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0  0x00000039212324f5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x0000003921233cd5 in abort () at abort.c:92
#2  0x0000000000acd16a in ExceptionalCondition (conditionName=0xc32310
"numfks == attmap->maplen", errorType=0xc2ea23 "FailedAssertion",
fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at assert.c:67
#3  0x00000000006d1b6c in CloneFkReferenced (parentRel=0x7f3c80be2400,
partitionRel=0x7f3c80be2a50) at tablecmds.c:9046
#4  0x00000000006d189b in CloneForeignKeyConstraints (wqueue=0x0,
parentRel=0x7f3c80be2400, partitionRel=0x7f3c80be2a50) at tablecmds.c:8939
#5  0x00000000006c09a8 in DefineRelation (stmt=0x2ff25b8, relkind=114 'r',
ownerId=10, typaddress=0x0, queryString=0x2f19810 "CREATE TABLE part1_p1
PARTITION OF part1 FOR VALUES IN (1);")
    at tablecmds.c:1151
#6  0x0000000000953021 in ProcessUtilitySlow (pstate=0x2ff24a0,
pstmt=0x2f1a588, queryString=0x2f19810 "CREATE TABLE part1_p1 PARTITION OF
part1 FOR VALUES IN (1);", context=PROCESS_UTILITY_TOPLEVEL,
    params=0x0, queryEnv=0x0, dest=0x2f1a868, qc=0x7ffffc1faa10) at
utility.c:1154
#7  0x0000000000952dfe in standard_ProcessUtility (pstmt=0x2f1a588,
queryString=0x2f19810 "CREATE TABLE part1_p1 PARTITION OF part1 FOR VALUES
IN (1);", context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
    queryEnv=0x0, dest=0x2f1a868, qc=0x7ffffc1faa10) at utility.c:1067
#8  0x0000000000951d18 in ProcessUtility (pstmt=0x2f1a588,
queryString=0x2f19810 "CREATE TABLE part1_p1 PARTITION OF part1 FOR VALUES
IN (1);", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
    dest=0x2f1a868, qc=0x7ffffc1faa10) at utility.c:522
#9  0x0000000000950b48 in PortalRunUtility (portal=0x2f808c0,
pstmt=0x2f1a588, isTopLevel=true, setHoldSnapshot=false, dest=0x2f1a868,
qc=0x7ffffc1faa10) at pquery.c:1157
#10 0x0000000000950d6e in PortalRunMulti (portal=0x2f808c0,
isTopLevel=true, setHoldSnapshot=false, dest=0x2f1a868, altdest=0x2f1a868,
qc=0x7ffffc1faa10) at pquery.c:1303
#11 0x000000000095023a in PortalRun (portal=0x2f808c0,
count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x2f1a868,
altdest=0x2f1a868, qc=0x7ffffc1faa10) at pquery.c:779
#12 0x000000000094a2a3 in exec_simple_query (query_string=0x2f19810 "CREATE
TABLE part1_p1 PARTITION OF part1 FOR VALUES IN (1);") at postgres.c:1239
#13 0x000000000094e38e in PostgresMain (argc=1, argv=0x2f44998,
dbname=0x2f448b0 "postgres", username=0x2f44890 "edb") at postgres.c:4315
#14 0x000000000089ba5d in BackendRun (port=0x2f3c7f0) at postmaster.c:4510
#15 0x000000000089b24c in BackendStartup (port=0x2f3c7f0) at
postmaster.c:4202
#16 0x00000000008975be in ServerLoop () at postmaster.c:1727
#17 0x0000000000896f07 in PostmasterMain (argc=3, argv=0x2f14240) at
postmaster.c:1400
#18 0x00000000007999cc in main (argc=3, argv=0x2f14240) at main.c:210

Thanks & Regards,
Rajkumar Raghuwanshi

  2. Re: create partition table caused server crashed with self-referencing foreign key

    Amul Sul <sulamul@gmail.com> — 2020-04-22T08:10:50Z

    On Wed, Apr 22, 2020 at 1:21 PM Rajkumar Raghuwanshi <
rajkumar.raghuwanshi@enterprisedb.com> wrote:

> Hi,
>
> Getting a server crash while creating partition table which  have
> self-referencing foreign key
>
> postgres=# CREATE TABLE part1 (c1 int PRIMARY KEY, c2 int REFERENCES
> part1) PARTITION BY LIST (c1);
> CREATE TABLE
> postgres=# CREATE TABLE part1_p1 PARTITION OF part1 FOR VALUES IN (1);
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>
> --stack-trace
> [edb@localhost bin]$ gdb -q -c data/core.16883 postgres
> Core was generated by `postgres: edb postgres [local] CREATE TABLE
>     '.
> Program terminated with signal 6, Aborted.
> #0  0x00000039212324f5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 64  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> Missing separate debuginfos, use: debuginfo-install
> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
> libcom_err-1.41.12-24.el6.x86_64 libgcc-4.4.7-23.el6.x86_64
> libselinux-2.0.94-7.el6.x86_64 openssl-1.0.1e-58.el6_10.x86_64
> zlib-1.2.3-29.el6.x86_64
> (gdb) bt
> #0  0x00000039212324f5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1  0x0000003921233cd5 in abort () at abort.c:92
> #2  0x0000000000acd16a in ExceptionalCondition (conditionName=0xc32310
> "numfks == attmap->maplen", errorType=0xc2ea23 "FailedAssertion",
> fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at assert.c:67
>

Looks like this assertion is incorrect, I guess it should have check
numfks <= attmap->maplen instead.

Regards,
Amul

  3. Re: create partition table caused server crashed with self-referencing foreign key

    David Rowley <dgrowleyml@gmail.com> — 2020-04-22T08:57:34Z

    On Wed, 22 Apr 2020 at 20:11, amul sul <sulamul@gmail.com> wrote:
>
> On Wed, Apr 22, 2020 at 1:21 PM Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote:
>> #2  0x0000000000acd16a in ExceptionalCondition (conditionName=0xc32310 "numfks == attmap->maplen", errorType=0xc2ea23 "FailedAssertion", fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at assert.c:67
>
>
> Looks like this assertion is incorrect, I guess it should have check
> numfks <= attmap->maplen instead.

Even that seems like a very strange thing to Assert. Basically it's
saying, make sure the number of columns in the foreign key constraint
is less than or equal to the number of attributes in parentRel.

It's true we do disallow duplicate column names in the foreign key
constraint (at least since 9da867537), but why do we want an Assert to
say that?  I don't see anything about that code that would break if we
did happen to allow duplicate columns in the foreign key.  I'd say the
Assert should just be removed completely.

David

  4. Re: create partition table caused server crashed with self-referencing foreign key

    Amul Sul <sulamul@gmail.com> — 2020-04-22T09:29:38Z

    On Wed, Apr 22, 2020 at 2:27 PM David Rowley <dgrowleyml@gmail.com> wrote:

> On Wed, 22 Apr 2020 at 20:11, amul sul <sulamul@gmail.com> wrote:
> >
> > On Wed, Apr 22, 2020 at 1:21 PM Rajkumar Raghuwanshi <
> rajkumar.raghuwanshi@enterprisedb.com> wrote:
> >> #2  0x0000000000acd16a in ExceptionalCondition (conditionName=0xc32310
> "numfks == attmap->maplen", errorType=0xc2ea23 "FailedAssertion",
> fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at assert.c:67
> >
> >
> > Looks like this assertion is incorrect, I guess it should have check
> > numfks <= attmap->maplen instead.
>
> Even that seems like a very strange thing to Assert. Basically it's
> saying, make sure the number of columns in the foreign key constraint
> is less than or equal to the number of attributes in parentRel.
>
> It's true we do disallow duplicate column names in the foreign key
> constraint (at least since 9da867537), but why do we want an Assert to
> say that?  I don't see anything about that code that would break if we
> did happen to allow duplicate columns in the foreign key.  I'd say the
> Assert should just be removed completely.
>

Understood and agree with you.

Attached patch removes this assertion and does a slight tweak to regression
test
to generate case where numfks != attmap->maplen, IMO, we should have this
even if there is nothing that checks it. Thoughts?

Regards,
Amul

  5. Re: create partition table caused server crashed with self-referencing foreign key

    Amul Sul <sulamul@gmail.com> — 2020-04-22T09:44:27Z

    On Wed, Apr 22, 2020 at 2:59 PM amul sul <sulamul@gmail.com> wrote:

>
>
> On Wed, Apr 22, 2020 at 2:27 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
>> On Wed, 22 Apr 2020 at 20:11, amul sul <sulamul@gmail.com> wrote:
>> >
>> > On Wed, Apr 22, 2020 at 1:21 PM Rajkumar Raghuwanshi <
>> rajkumar.raghuwanshi@enterprisedb.com> wrote:
>> >> #2  0x0000000000acd16a in ExceptionalCondition (conditionName=0xc32310
>> "numfks == attmap->maplen", errorType=0xc2ea23 "FailedAssertion",
>> fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at assert.c:67
>> >
>> >
>> > Looks like this assertion is incorrect, I guess it should have check
>> > numfks <= attmap->maplen instead.
>>
>> Even that seems like a very strange thing to Assert. Basically it's
>> saying, make sure the number of columns in the foreign key constraint
>> is less than or equal to the number of attributes in parentRel.
>>
>> It's true we do disallow duplicate column names in the foreign key
>> constraint (at least since 9da867537), but why do we want an Assert to
>> say that?  I don't see anything about that code that would break if we
>> did happen to allow duplicate columns in the foreign key.  I'd say the
>> Assert should just be removed completely.
>>
>
> Understood and agree with you.
>
> Attached patch removes this assertion and does a slight tweak to
> regression test
> to generate case where numfks != attmap->maplen, IMO, we should have this
> even if there is nothing that checks it. Thoughts?
>

Kindly ignore the previously attached patch, correct patch attached here.

Regards,
Amul

  6. Re: create partition table caused server crashed with self-referencing foreign key

    David Rowley <dgrowleyml@gmail.com> — 2020-04-22T10:21:21Z

    On Wed, 22 Apr 2020 at 21:30, amul sul <sulamul@gmail.com> wrote:
>
> On Wed, Apr 22, 2020 at 2:27 PM David Rowley <dgrowleyml@gmail.com> wrote:
>>
>> On Wed, 22 Apr 2020 at 20:11, amul sul <sulamul@gmail.com> wrote:
>> >
>> > On Wed, Apr 22, 2020 at 1:21 PM Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote:
>> >> #2  0x0000000000acd16a in ExceptionalCondition (conditionName=0xc32310 "numfks == attmap->maplen", errorType=0xc2ea23 "FailedAssertion", fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at assert.c:67
>> >
>> >
>> > Looks like this assertion is incorrect, I guess it should have check
>> > numfks <= attmap->maplen instead.
>>
>> Even that seems like a very strange thing to Assert. Basically it's
>> saying, make sure the number of columns in the foreign key constraint
>> is less than or equal to the number of attributes in parentRel.
>>
>> It's true we do disallow duplicate column names in the foreign key
>> constraint (at least since 9da867537), but why do we want an Assert to
>> say that?  I don't see anything about that code that would break if we
>> did happen to allow duplicate columns in the foreign key.  I'd say the
>> Assert should just be removed completely.
>
>
> Understood and agree with you.

I pushed a patch to remove the Assert. I didn't really feel a need to
make any adjustments to the regression tests for this. The Assert was
clearly out of place, it's hard to imagine that this could ever get
broken again.

David

  7. Re: create partition table caused server crashed with self-referencing foreign key

    Ahsan Hadi <ahsan.hadi@gmail.com> — 2020-04-22T10:44:07Z

    On Wed, Apr 22, 2020 at 2:45 PM amul sul <sulamul@gmail.com> wrote:

>
>
> On Wed, Apr 22, 2020 at 2:59 PM amul sul <sulamul@gmail.com> wrote:
>
>>
>>
>> On Wed, Apr 22, 2020 at 2:27 PM David Rowley <dgrowleyml@gmail.com>
>> wrote:
>>
>>> On Wed, 22 Apr 2020 at 20:11, amul sul <sulamul@gmail.com> wrote:
>>> >
>>> > On Wed, Apr 22, 2020 at 1:21 PM Rajkumar Raghuwanshi <
>>> rajkumar.raghuwanshi@enterprisedb.com> wrote:
>>> >> #2  0x0000000000acd16a in ExceptionalCondition
>>> (conditionName=0xc32310 "numfks == attmap->maplen", errorType=0xc2ea23
>>> "FailedAssertion", fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at
>>> assert.c:67
>>> >
>>> >
>>> > Looks like this assertion is incorrect, I guess it should have check
>>> > numfks <= attmap->maplen instead.
>>>
>>> Even that seems like a very strange thing to Assert. Basically it's
>>> saying, make sure the number of columns in the foreign key constraint
>>> is less than or equal to the number of attributes in parentRel.
>>>
>>> It's true we do disallow duplicate column names in the foreign key
>>> constraint (at least since 9da867537), but why do we want an Assert to
>>> say that?  I don't see anything about that code that would break if we
>>> did happen to allow duplicate columns in the foreign key.  I'd say the
>>> Assert should just be removed completely.
>>>
>>
>> Understood and agree with you.
>>
>> Attached patch removes this assertion and does a slight tweak to
>> regression test
>> to generate case where numfks != attmap->maplen, IMO, we should have this
>> even if there is nothing that checks it. Thoughts?
>>
>
> Kindly ignore the previously attached patch, correct patch attached here.
>

I did a quick test of the fix, the assertion failure is fixed and
regression is not reporting any failures.


>
> Regards,
> Amul
>


-- 
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.hadi@highgo.ca

  8. Re: create partition table caused server crashed with self-referencing foreign key

    Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> — 2020-04-22T11:34:04Z

    Thanks all for quick fix and push.

Thanks & Regards,
Rajkumar Raghuwanshi


On Wed, Apr 22, 2020 at 4:14 PM Ahsan Hadi <ahsan.hadi@gmail.com> wrote:

>
>
> On Wed, Apr 22, 2020 at 2:45 PM amul sul <sulamul@gmail.com> wrote:
>
>>
>>
>> On Wed, Apr 22, 2020 at 2:59 PM amul sul <sulamul@gmail.com> wrote:
>>
>>>
>>>
>>> On Wed, Apr 22, 2020 at 2:27 PM David Rowley <dgrowleyml@gmail.com>
>>> wrote:
>>>
>>>> On Wed, 22 Apr 2020 at 20:11, amul sul <sulamul@gmail.com> wrote:
>>>> >
>>>> > On Wed, Apr 22, 2020 at 1:21 PM Rajkumar Raghuwanshi <
>>>> rajkumar.raghuwanshi@enterprisedb.com> wrote:
>>>> >> #2  0x0000000000acd16a in ExceptionalCondition
>>>> (conditionName=0xc32310 "numfks == attmap->maplen", errorType=0xc2ea23
>>>> "FailedAssertion", fileName=0xc2f0bf "tablecmds.c", lineNumber=9046) at
>>>> assert.c:67
>>>> >
>>>> >
>>>> > Looks like this assertion is incorrect, I guess it should have check
>>>> > numfks <= attmap->maplen instead.
>>>>
>>>> Even that seems like a very strange thing to Assert. Basically it's
>>>> saying, make sure the number of columns in the foreign key constraint
>>>> is less than or equal to the number of attributes in parentRel.
>>>>
>>>> It's true we do disallow duplicate column names in the foreign key
>>>> constraint (at least since 9da867537), but why do we want an Assert to
>>>> say that?  I don't see anything about that code that would break if we
>>>> did happen to allow duplicate columns in the foreign key.  I'd say the
>>>> Assert should just be removed completely.
>>>>
>>>
>>> Understood and agree with you.
>>>
>>> Attached patch removes this assertion and does a slight tweak to
>>> regression test
>>> to generate case where numfks != attmap->maplen, IMO, we should have
>>> this
>>> even if there is nothing that checks it. Thoughts?
>>>
>>
>> Kindly ignore the previously attached patch, correct patch attached here.
>>
>
> I did a quick test of the fix, the assertion failure is fixed and
> regression is not reporting any failures.
>
>
>>
>> Regards,
>> Amul
>>
>
>
> --
> Highgo Software (Canada/China/Pakistan)
> URL : http://www.highgo.ca
> ADDR: 10318 WHALLEY BLVD, Surrey, BC
> EMAIL: mailto: ahsan.hadi@highgo.ca
>

  9. Re: create partition table caused server crashed with self-referencing foreign key

    Michael Paquier <michael@paquier.xyz> — 2020-04-22T11:50:51Z

    On Wed, Apr 22, 2020 at 10:21:21PM +1200, David Rowley wrote:
> I pushed a patch to remove the Assert. I didn't really feel a need to
> make any adjustments to the regression tests for this. The Assert was
> clearly out of place, it's hard to imagine that this could ever get
> broken again.

Still, it seems to me that there could be a point in having a test for
partitioned tables with FKs referencing themselves.  We have such
tests for plain tables for example.
--
Michael

  10. Re: create partition table caused server crashed with self-referencing foreign key

    David Rowley <dgrowleyml@gmail.com> — 2020-04-23T02:50:22Z

    On Wed, 22 Apr 2020 at 23:50, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Apr 22, 2020 at 10:21:21PM +1200, David Rowley wrote:
> > I pushed a patch to remove the Assert. I didn't really feel a need to
> > make any adjustments to the regression tests for this. The Assert was
> > clearly out of place, it's hard to imagine that this could ever get
> > broken again.
>
> Still, it seems to me that there could be a point in having a test for
> partitioned tables with FKs referencing themselves.  We have such
> tests for plain tables for example.

The reason I didn't take the additional tests that were proposed by
Amul was that I didn't think that they really added any additional
coverage to the code that remained.  They would only have served to
ensure that nobody went and added the same Assert back again. Since
the Assert was clearly out of place, I didn't think it was worthy of
burdening our build farm with the additional overhead of the modified
test from now until the end of time.

I'd put it akin to fixing a spelling mistake in an error message and
adding a special test specifically to ensure the spelling is correct.
Would we add a test for that? Likely not. Would someone reintroduce
the spelling mistake again? Likely not.

I think discussions for any tests beyond the scope of this fix are
probably for another thread. I'm happy to join any discussions about
it there.

David