On Tue, Jan 24, 2012 at 4:03 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Phil Sorber <p...@omniti.com> writes:
>> On Tue, Jan 24, 2012 at 12:43 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> How about a test case?
>
>> We are having trouble coming up with a test case that can reliably
>> reproduce this.
>
> The stack traces run through the EvalPlanQual machinery, which is only
> going to be entered when attempting to update/delete a row that's been
> updated by a concurrent transaction. So what I'd suggest for creating a
> test case is
>
> (1) in a psql session, do
> BEGIN;
> UPDATE some-target-row;
>
> (2) in another psql session, call this function with arguments
> that will make it try to update that same row; it should
> block.
>
> (3) in the first session, COMMIT to unblock.
>
That helped a lot. I now have a simple test case that I can reliably
re-produce the segfault and now also a patch that fixes it. I had to
modify the patch slightly because while it fixed the first problem, it
just cascaded to another NULL deref from the same root cause. Both are
attached.
> FWIW, having re-examined your patch with some caffeine in me, I don't
> think it's right at all. You can't just blow off setting the scan type
> for a CTEScan node. What it looks like to me is that the EvalPlanQual
> code is not initializing the new execution tree correctly; but that
> idea would be a lot easier to check into with a test case.
>
If I understand what you are saying, then I agree that is the root of
the problem. The comment label's it as an optimization, but then later
fails to account for all the changes needed. My patch accounts for at
least one extra change that is needed. We could also remove the
"optimization" but I assumed it was there for a reason, especially
given the fact that someone took the time to make a comment about it.
The change was made in this commit by you:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;f=src/backend/executor/execMain.c;h=389af951552ff2209eae3e62fa147fef12329d4f
> regards, tom lane
create table t1 (id uuid, data text, primary key (id));
WITH upsert AS
(UPDATE
t1
SET
data = 'test'
WHERE
id='ad400a94-ad7a-4375-92c6-7294e3e4ce6d'
RETURNING
id)
INSERT INTO
user_data (id,
data)
SELECT
'ad400a94-ad7a-4375-92c6-7294e3e4ce6d',
'test'
WHERE
NOT EXISTS
(SELECT
true
FROM
upsert);
BEGIN;
WITH upsert AS
(UPDATE
t1
SET
data = 'test'
WHERE
id='ad400a94-ad7a-4375-92c6-7294e3e4ce6d'
RETURNING
id)
INSERT INTO
user_data (id,
data)
SELECT
'ad400a94-ad7a-4375-92c6-7294e3e4ce6d',
'test'
WHERE
NOT EXISTS
(SELECT
true
FROM
upsert);
-- In separate session
WITH upsert AS
(UPDATE
t1
SET
data = 'test'
WHERE
id='ad400a94-ad7a-4375-92c6-7294e3e4ce6d'
RETURNING
id)
INSERT INTO
user_data (id,
data)
SELECT
'ad400a94-ad7a-4375-92c6-7294e3e4ce6d',
'test'
WHERE
NOT EXISTS
(SELECT
true
FROM
upsert);
-- In original session
END;
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
new file mode 100644
index 1bb1094..dcec11e
*** a/src/backend/executor/execScan.c
--- b/src/backend/executor/execScan.c
*************** ExecScan(ScanState *node,
*** 245,253 ****
void
ExecAssignScanProjectionInfo(ScanState *node)
{
! Scan *scan = (Scan *) node->ps.plan;
Index varno;
/* Vars in an index-only scan's tlist should be INDEX_VAR */
if (IsA(scan, IndexOnlyScan))
varno = INDEX_VAR;
--- 245,256 ----
void
ExecAssignScanProjectionInfo(ScanState *node)
{
! Scan *scan;
Index varno;
+ Assert(node != NULL);
+ scan = (Scan *) node->ps.plan;
+
/* Vars in an index-only scan's tlist should be INDEX_VAR */
if (IsA(scan, IndexOnlyScan))
varno = INDEX_VAR;
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
new file mode 100644
index 6db42e7..f285dc0
*** a/src/backend/executor/execUtils.c
--- b/src/backend/executor/execUtils.c
*************** ExecAssignResultTypeFromTL(PlanState *pl
*** 448,453 ****
--- 448,454 ----
bool hasoid;
TupleDesc tupDesc;
+ Assert(planstate != NULL);
if (ExecContextForcesOids(planstate, &hasoid))
{
/* context forces OID choice; hasoid is now set correctly */
*************** ExecAssignResultTypeFromTL(PlanState *pl
*** 474,480 ****
TupleDesc
ExecGetResultType(PlanState *planstate)
{
! TupleTableSlot *slot = planstate->ps_ResultTupleSlot;
return slot->tts_tupleDescriptor;
}
--- 475,484 ----
TupleDesc
ExecGetResultType(PlanState *planstate)
{
! TupleTableSlot *slot;
!
! Assert(planstate != NULL);
! slot = planstate->ps_ResultTupleSlot;
return slot->tts_tupleDescriptor;
}
diff --git a/src/backend/executor/nodeCtescan.c b/src/backend/executor/nodeCtescan.c
new file mode 100644
index 7ab15ac..ab8bfa3
*** a/src/backend/executor/nodeCtescan.c
--- b/src/backend/executor/nodeCtescan.c
*************** ExecInitCteScan(CteScan *node, EState *e
*** 255,271 ****
/*
* The scan tuple type (ie, the rowtype we expect to find in the work
* table) is the same as the result rowtype of the CTE query.
*/
! ExecAssignScanType(&scanstate->ss,
ExecGetResultType(scanstate->cteplanstate));
! /*
! * Initialize result tuple type and projection info.
! */
! ExecAssignResultTypeFromTL(&scanstate->ss.ps);
! ExecAssignScanProjectionInfo(&scanstate->ss);
! scanstate->ss.ps.ps_TupFromTlist = false;
return scanstate;
}
--- 255,276 ----
/*
* The scan tuple type (ie, the rowtype we expect to find in the work
* table) is the same as the result rowtype of the CTE query.
+ * We must check for NULL because we don't initialize data-modifying
+ * CTE's with a ModifyTable node at the top.
*/
! if (scanstate->cteplanstate != NULL)
! {
! ExecAssignScanType(&scanstate->ss,
ExecGetResultType(scanstate->cteplanstate));
! /*
! * Initialize result tuple type and projection info.
! */
! ExecAssignResultTypeFromTL(&scanstate->ss.ps);
! ExecAssignScanProjectionInfo(&scanstate->ss);
! scanstate->ss.ps.ps_TupFromTlist = false;
! }
return scanstate;
}
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
