Thread: BUG #17076: Server crashes on composing an error message about invalid modulus for a new table partition

The following bug has been logged on the website:

Bug reference:      17076
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 14beta2
Operating system:   Ubuntu 20.04
Description:

When executing the following query (based on create_table.sql):
CREATE TABLE hash_parted (a int) PARTITION BY HASH (a);
CREATE TABLE hpart_1 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 10,
REMAINDER 0);
CREATE TABLE hpart_3 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 200,
REMAINDER 2);
CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
150, REMAINDER 3);

Server crashes with the following stack:
Core was generated by `postgres: law regression [local] CREATE TABLE
                        '.
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f032f726859 in __GI_abort () at abort.c:79
#2  0x00005577bdce1ea3 in ExceptionalCondition
(conditionName=conditionName@entry=0x5577bdef056c "strvalue != NULL", 
    errorType=errorType@entry=0x5577bdd4100b "FailedAssertion",
fileName=0x7ffe0bcd1ac0 "\204\036\316\275wU", 
    fileName@entry=0x5577bdef0561 "snprintf.c",
lineNumber=lineNumber@entry=442) at assert.c:69
#3  0x00005577bdd3f04c in dopr (target=target@entry=0x7ffe0bcd20a0,
format=0x5577bde49b5f "\".", 
    format@entry=0x5577bde49b10 "The new modulus %d is not a factor of %d,
the modulus of existing partition \"%s\".", 
    args=0x7ffe0bcd2150) at snprintf.c:442
#4  0x00005577bdd3f71f in pg_vsnprintf (str=<optimized out>,
count=<optimized out>, count@entry=1024, 
    fmt=fmt@entry=0x5577bde49b10 "The new modulus %d is not a factor of %d,
the modulus of existing partition \"%s\".", 
    args=args@entry=0x7ffe0bcd2150) at snprintf.c:195
#5  0x00005577bdd359f6 in pvsnprintf (buf=<optimized out>,
len=len@entry=1024, 
    fmt=fmt@entry=0x5577bde49b10 "The new modulus %d is not a factor of %d,
the modulus of existing partition \"%s\".", 
    args=args@entry=0x7ffe0bcd2150) at psprintf.c:110
#6  0x00005577bdd36de4 in appendStringInfoVA (str=str@entry=0x7ffe0bcd2130,

    fmt=fmt@entry=0x5577bde49b10 "The new modulus %d is not a factor of %d,
the modulus of existing partition \"%s\".", 
    args=args@entry=0x7ffe0bcd2150) at stringinfo.c:149
#7  0x00005577bdce7042 in errdetail (
    fmt=fmt@entry=0x5577bde49b10 "The new modulus %d is not a factor of %d,
the modulus of existing partition \"%s\".")
    at elog.c:1051
#8  0x00005577bdaec445 in check_new_partition_bound
(relname=relname@entry=0x7ffe0bcd24a0 "fail_part", 
    parent=parent@entry=0x7f03223053b0, spec=spec@entry=0x5577bf9e9458,
pstate=pstate@entry=0x5577bf9e8188)
    at partbounds.c:2902
#9  0x00005577bd9b068b in DefineRelation (stmt=stmt@entry=0x5577bf8fb3c8,
relkind=relkind@entry=114 'r', ownerId=10, 
    ownerId@entry=0, typaddress=typaddress@entry=0x0, 
    queryString=queryString@entry=0x5577bf8fa670 "CREATE TABLE fail_part
PARTITION OF hash_parted FOR VALUES WITH (MODULUS 150, REMAINDER 3);") at
tablecmds.c:1059
#10 0x00005577bdbad002 in ProcessUtilitySlow
(pstate=pstate@entry=0x5577bf91bf80, pstmt=pstmt@entry=0x5577bf8fb770, 
    queryString=queryString@entry=0x5577bf8fa670 "CREATE TABLE fail_part
PARTITION OF hash_parted FOR VALUES WITH (MODULUS 150, REMAINDER 3);",
context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=params@entry=0x0, 
    queryEnv=queryEnv@entry=0x0, dest=0x5577bf8fb840, qc=0x7ffe0bcd2ad0) at
utility.c:1146
#11 0x00005577bdbacd34 in standard_ProcessUtility (pstmt=0x5577bf8fb770, 
    queryString=0x5577bf8fa670 "CREATE TABLE fail_part PARTITION OF
hash_parted FOR VALUES WITH (MODULUS 150, REMAINDER 3);",
readOnlyTree=<optimized out>, context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
queryEnv=0x0, dest=0x5577bf8fb840, 
    qc=0x7ffe0bcd2ad0) at utility.c:1049
#12 0x00005577bdbace1d in ProcessUtility (pstmt=pstmt@entry=0x5577bf8fb770,
queryString=<optimized out>, 
    readOnlyTree=<optimized out>,
context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=<optimized out>, 
    queryEnv=<optimized out>, dest=0x5577bf8fb840, qc=0x7ffe0bcd2ad0) at
utility.c:527
#13 0x00005577bdbaa343 in PortalRunUtility
(portal=portal@entry=0x5577bf969570, pstmt=pstmt@entry=0x5577bf8fb770, 
    isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x5577bf8fb840,

    qc=qc@entry=0x7ffe0bcd2ad0) at pquery.c:1147
#14 0x00005577bdbaa645 in PortalRunMulti
(portal=portal@entry=0x5577bf969570, isTopLevel=isTopLevel@entry=true, 
    setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x5577bf8fb840, altdest=altdest@entry=0x5577bf8fb840, 
    qc=qc@entry=0x7ffe0bcd2ad0) at pquery.c:1304
#15 0x00005577bdbaaa79 in PortalRun (portal=portal@entry=0x5577bf969570,
count=count@entry=9223372036854775807, 
    isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
dest=dest@entry=0x5577bf8fb840, 
    altdest=altdest@entry=0x5577bf8fb840, qc=0x7ffe0bcd2ad0) at
pquery.c:786
#16 0x00005577bdba6cc6 in exec_simple_query (
    query_string=query_string@entry=0x5577bf8fa670 "CREATE TABLE fail_part
PARTITION OF hash_parted FOR VALUES WITH (MODULUS 150, REMAINDER 3);") at
postgres.c:1214
#17 0x00005577bdba8c98 in PostgresMain (argc=argc@entry=1,
argv=argv@entry=0x7ffe0bcd2cc0, dbname=<optimized out>, 
    username=<optimized out>) at postgres.c:4486
#18 0x00005577bdb03b65 in BackendRun (port=port@entry=0x5577bf91dee0) at
postmaster.c:4507
#19 0x00005577bdb06d7a in BackendStartup (port=port@entry=0x5577bf91dee0) at
postmaster.c:4229
#20 0x00005577bdb06fc1 in ServerLoop () at postmaster.c:1745
#21 0x00005577bdb0850e in PostmasterMain (argc=3, argv=<optimized out>) at
postmaster.c:1417
#22 0x00005577bda49158 in main (argc=3, argv=0x5577bf8f4930) at main.c:209

PG Bug reporting form <noreply@postgresql.org> writes:
> When executing the following query (based on create_table.sql):
> CREATE TABLE hash_parted (a int) PARTITION BY HASH (a);
> CREATE TABLE hpart_1 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 10,
> REMAINDER 0);
> CREATE TABLE hpart_3 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 200,
> REMAINDER 2);
> CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
> 150, REMAINDER 3);

> Server crashes with the following stack:

So this is evidently the fault of efbfb6424, which isn't accounting
for the possibility that the next remainder has no associated partition.
That results in boundinfo->indexes[offset + 1] being -1, whereupon we
pass garbage to get_rel_name() and get back a NULL.

I think the patch has got more problems than that too, as it's far
from clear why the next remainder would have anything to do with the
next larger modulus.  IOW I suspect that when it does manage to print
a partition name without crashing, it's very likely to print the
wrong partition.

I'm inclined to think we should just revert efbfb6424, and maybe
try again some other time.

            regards, tom lane

I wrote:
> I think the patch has got more problems than that too, as it's far
> from clear why the next remainder would have anything to do with the
> next larger modulus.  IOW I suspect that when it does manage to print
> a partition name without crashing, it's very likely to print the
> wrong partition.

Concretely, this variant of the test case:

drop table hash_parted;
CREATE TABLE hash_parted (
    a int
) PARTITION BY HASH (a);
CREATE TABLE hpart_1 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 50, REMAINDER 0);
CREATE TABLE hpart_2 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 10, REMAINDER 1);
CREATE TABLE hpart_3 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 200, REMAINDER 2);

CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 25, REMAINDER 3);

prints

ERROR:  every hash partition modulus must be a factor of the next larger modulus
DETAIL:  The new modulus 25 is not divisible by 10, the modulus of existing partition "hpart_1".

which is obviously wrong.

I think that rescuing this might require scanning through the list of
partitions for one that has the desired modulus.  There could be more
than one match, but I'd be content to take the first one.

            regards, tom lane

On Tue, Jun 29, 2021 at 7:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > I think the patch has got more problems than that too, as it's far
> > from clear why the next remainder would have anything to do with the
> > next larger modulus.  IOW I suspect that when it does manage to print
> > a partition name without crashing, it's very likely to print the
> > wrong partition.
>
> Concretely, this variant of the test case:
>
> drop table hash_parted;
> CREATE TABLE hash_parted (
>         a int
> ) PARTITION BY HASH (a);
> CREATE TABLE hpart_1 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 50, REMAINDER 0);
> CREATE TABLE hpart_2 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 10, REMAINDER 1);
> CREATE TABLE hpart_3 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 200, REMAINDER 2);
>
> CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 25, REMAINDER 3);
>
> prints
>
> ERROR:  every hash partition modulus must be a factor of the next larger modulus
> DETAIL:  The new modulus 25 is not divisible by 10, the modulus of existing partition "hpart_1".
>
> which is obviously wrong.

Yes.  Though, I think the problem is that the code uses
boundinfo->indexes[<offset>] as an index into partdesc->oids[] array
to get the existing partition to show in the error message.  The
correct thing here would be to use <offset> directly.  That is because
the hash partition bounds as laid out in boudinfo->datums[] correspond
one-to-one with the partition OIDs laid out in partdesc->oids[].
boundinfo->indexes should only be consulted to get the partition OID
from a remainder value, as get_partition_for_tuple() does.

(OTOH, range and list partition bound offsets, because there may be
more than one bound assigned to each partition (considering
de-duplication of pairs of matching upper and next lower bound in the
range case), do not correspond one-to-one with those of partition
OIDs.)

> I think that rescuing this might require scanning through the list of
> partitions for one that has the desired modulus.  There could be more
> than one match, but I'd be content to take the first one.

Hmm, the bound offset that the code comes up with to show in the error
message seems fine to me.

I have attached a patch containing the above mentioned fix.  The patch
also adjusts the relevant test cases in create_table.sql to cover
these cases.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

Amit Langote <amitlangote09@gmail.com> writes:
> On Tue, Jun 29, 2021 at 7:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think the patch has got more problems than that too, as it's far
>>> from clear why the next remainder would have anything to do with the
>>> next larger modulus.

> Yes.  Though, I think the problem is that the code uses
> boundinfo->indexes[<offset>] as an index into partdesc->oids[] array
> to get the existing partition to show in the error message.  The
> correct thing here would be to use <offset> directly.  That is because
> the hash partition bounds as laid out in boudinfo->datums[] correspond
> one-to-one with the partition OIDs laid out in partdesc->oids[].

Got it.  Pushed your fix, along with some twiddling of the nearby
comments which seemed a bit vague.

            regards, tom lane