Обсуждение: runtime error copying oids field

Поиск
Список
Период
Сортировка

runtime error copying oids field

От
Zhihong Yu
Дата:
Hi,
In our testPgRegressTrigger test log, I saw the following (this was for a relatively old version of PG):

197859  [ts-1]  ../../../../../../src/postgres/src/backend/commands/indexcmds.c:1062:22: runtime error: null pointer passed as argument 2, which is declared to never be null
197860  [ts-1]  /opt/yb-build/brew/linuxbrew-20181203T161736v9/include/string.h:43:28: note: nonnull attribute specified here
197861  [ts-1]       #0 0xacbd0f in DefineIndex $YB_SRC_ROOT/src/postgres/src/backend/commands/../../../../../../src/postgres/src/backend/commands/indexcmds.c:1062:4
197862  [ts-1]       #1 0x11441e0 in ProcessUtilitySlow $YB_SRC_ROOT/src/postgres/src/backend/tcop/../../../../../../src/postgres/src/backend/tcop/utility.c:1436:7
197863  [ts-1]       #2 0x114141f in standard_ProcessUtility $YB_SRC_ROOT/src/postgres/src/backend/tcop/../../../../../../src/postgres/src/backend/tcop/utility.c:962:4
197864  [ts-1]       #3 0x1140b65 in YBProcessUtilityDefaultHook $YB_SRC_ROOT/src/postgres/src/backend/tcop/../../../../../../src/postgres/src/backend/tcop/utility.c:3574:3
197865  [ts-1]       #4 0x7f47d4950eac in pgss_ProcessUtility $YB_SRC_ROOT/src/postgres/contrib/pg_stat_statements/../../../../../src/postgres/contrib/pg_stat_statements/pg_stat_statements.c:1120:4

This was the line runtime error was raised:

        memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);

From RelationBuildPartitionDesc we can see that:

	if (nparts > 0)	{		PartitionBoundInfo boundinfo;		int		   *mapping;		int			next_index = 0;
		result->oids = (Oid *) palloc0(nparts * sizeof(Oid));

The cause was oids field was not assigned due to nparts being 0.
This is verified by additional logging added just prior to the memcpy call.

I want to get the community's opinion on whether a null check should be added prior to the memcpy() call.

Cheers

Re: runtime error copying oids field

От
Alvaro Herrera
Дата:
On 2020-Nov-30, Zhihong Yu wrote:

> This was the line runtime error was raised:
> 
>         memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
> 
> From RelationBuildPartitionDesc we can see that:
> 
>     if (nparts > 0)
>     {
>         PartitionBoundInfo boundinfo;
>         int           *mapping;
>         int            next_index = 0;
> 
>         result->oids = (Oid *) palloc0(nparts * sizeof(Oid));
> 
> The cause was oids field was not assigned due to nparts being 0.
> This is verified by additional logging added just prior to the memcpy call.
> 
> I want to get the community's opinion on whether a null check should be
> added prior to the memcpy() call.

As far as I understand, we do want to avoid memcpy's of null pointers;
see [1].

In this case I think it'd be sane to skip the complete block, not just
the memcpy, something like

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca24620fd0..d35deb433a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1163,15 +1163,17 @@ DefineIndex(Oid relationId,
 
     if (partitioned)
     {
+        PartitionDesc partdesc;
+
         /*
          * Unless caller specified to skip this step (via ONLY), process each
          * partition to make sure they all contain a corresponding index.
          *
          * If we're called internally (no stmt->relation), recurse always.
          */
-        if (!stmt->relation || stmt->relation->inh)
+        partdesc = RelationGetPartitionDesc(rel);
+        if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
         {
-            PartitionDesc partdesc = RelationGetPartitionDesc(rel);
             int            nparts = partdesc->nparts;
             Oid           *part_oids = palloc(sizeof(Oid) * nparts);
             bool        invalidate_parent = false;

[1] https://www.postgresql.org/message-id/flat/20200904023648.GB3426768%40rfd.leadboat.com

Re: runtime error copying oids field

От
Zhihong Yu
Дата:
Hi,
See attached patch which is along the line Alvaro outlined.

Cheers

On Mon, Nov 30, 2020 at 3:01 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2020-Nov-30, Zhihong Yu wrote:

> This was the line runtime error was raised:
>
>         memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
>
> From RelationBuildPartitionDesc we can see that:
>
>       if (nparts > 0)
>       {
>               PartitionBoundInfo boundinfo;
>               int                *mapping;
>               int                     next_index = 0;
>
>               result->oids = (Oid *) palloc0(nparts * sizeof(Oid));
>
> The cause was oids field was not assigned due to nparts being 0.
> This is verified by additional logging added just prior to the memcpy call.
>
> I want to get the community's opinion on whether a null check should be
> added prior to the memcpy() call.

As far as I understand, we do want to avoid memcpy's of null pointers;
see [1].

In this case I think it'd be sane to skip the complete block, not just
the memcpy, something like

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca24620fd0..d35deb433a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1163,15 +1163,17 @@ DefineIndex(Oid relationId,

        if (partitioned)
        {
+               PartitionDesc partdesc;
+
                /*
                 * Unless caller specified to skip this step (via ONLY), process each
                 * partition to make sure they all contain a corresponding index.
                 *
                 * If we're called internally (no stmt->relation), recurse always.
                 */
-               if (!stmt->relation || stmt->relation->inh)
+               partdesc = RelationGetPartitionDesc(rel);
+               if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
                {
-                       PartitionDesc partdesc = RelationGetPartitionDesc(rel);
                        int                     nparts = partdesc->nparts;
                        Oid                *part_oids = palloc(sizeof(Oid) * nparts);
                        bool            invalidate_parent = false;

[1] https://www.postgresql.org/message-id/flat/20200904023648.GB3426768%40rfd.leadboat.com
Вложения

Re: runtime error copying oids field

От
Zhihong Yu
Дата:
Alvaro, et al:
Please let me know how to proceed with the patch.

Running test suite with the patch showed no regression.

Cheers

On Mon, Nov 30, 2020 at 3:24 PM Zhihong Yu <zyu@yugabyte.com> wrote:
Hi,
See attached patch which is along the line Alvaro outlined.

Cheers

On Mon, Nov 30, 2020 at 3:01 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2020-Nov-30, Zhihong Yu wrote:

> This was the line runtime error was raised:
>
>         memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
>
> From RelationBuildPartitionDesc we can see that:
>
>       if (nparts > 0)
>       {
>               PartitionBoundInfo boundinfo;
>               int                *mapping;
>               int                     next_index = 0;
>
>               result->oids = (Oid *) palloc0(nparts * sizeof(Oid));
>
> The cause was oids field was not assigned due to nparts being 0.
> This is verified by additional logging added just prior to the memcpy call.
>
> I want to get the community's opinion on whether a null check should be
> added prior to the memcpy() call.

As far as I understand, we do want to avoid memcpy's of null pointers;
see [1].

In this case I think it'd be sane to skip the complete block, not just
the memcpy, something like

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca24620fd0..d35deb433a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1163,15 +1163,17 @@ DefineIndex(Oid relationId,

        if (partitioned)
        {
+               PartitionDesc partdesc;
+
                /*
                 * Unless caller specified to skip this step (via ONLY), process each
                 * partition to make sure they all contain a corresponding index.
                 *
                 * If we're called internally (no stmt->relation), recurse always.
                 */
-               if (!stmt->relation || stmt->relation->inh)
+               partdesc = RelationGetPartitionDesc(rel);
+               if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
                {
-                       PartitionDesc partdesc = RelationGetPartitionDesc(rel);
                        int                     nparts = partdesc->nparts;
                        Oid                *part_oids = palloc(sizeof(Oid) * nparts);
                        bool            invalidate_parent = false;

[1] https://www.postgresql.org/message-id/flat/20200904023648.GB3426768%40rfd.leadboat.com

Re: runtime error copying oids field

От
Alvaro Herrera
Дата:
On 2020-Nov-30, Zhihong Yu wrote:

> Alvaro, et al:
> Please let me know how to proceed with the patch.
> 
> Running test suite with the patch showed no regression.

That's good to hear.  I'll get it pushed today.  Thanks for following
up.

Re: runtime error copying oids field

От
Alvaro Herrera
Дата:
On 2020-Dec-01, Alvaro Herrera wrote:

> On 2020-Nov-30, Zhihong Yu wrote:
> 
> > Alvaro, et al:
> > Please let me know how to proceed with the patch.
> > 
> > Running test suite with the patch showed no regression.
> 
> That's good to hear.  I'll get it pushed today.  Thanks for following
> up.

Done, thanks for reporting this.