runtime error copying oids field
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
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]/messages/by-id/20200904023648.GB3426768@rfd.leadboat.com.
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]: /messages/by-id/20200904023648.GB3426768@rfd.leadboat.com
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:
Show quoted text
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 memcpycall.
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 likediff --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]
/messages/by-id/20200904023648.GB3426768@rfd.leadboat.com
Attachments:
v01-check-nparts-for-defining-idx.patchapplication/octet-stream; name=v01-check-nparts-for-defining-idx.patchDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca24620fd0..7d5966bf9c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1169,9 +1169,9 @@ DefineIndex(Oid relationId,
*
* If we're called internally (no stmt->relation), recurse always.
*/
- if (!stmt->relation || stmt->relation->inh)
+ PartitionDesc 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;
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:
Show quoted text
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 memcpycall.
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 likediff --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]
/messages/by-id/20200904023648.GB3426768@rfd.leadboat.com
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.
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.