Assignment before assert

Started by Dmitry Koval11 months ago4 messages
#1Dmitry Koval
d.koval@postgrespro.ru
1 attachment(s)

Hi!
Function EvalPlanQualFetchRowMark contains an assignment

ExecRowMark *erm = earm->rowmark;

before assert

Assert(earm != NULL);

Maybe these lines need to be swapped?

--
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com

Attachments:

v1-0001-fix-assignment-before-assert.patchtext/plain; charset=UTF-8; name=v1-0001-fix-assignment-before-assert.patchDownload
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 39d80ccfbad..963aa390620 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2662,13 +2662,15 @@ bool
 EvalPlanQualFetchRowMark(EPQState *epqstate, Index rti, TupleTableSlot *slot)
 {
 	ExecAuxRowMark *earm = epqstate->relsubs_rowmark[rti - 1];
-	ExecRowMark *erm = earm->rowmark;
+	ExecRowMark *erm;
 	Datum		datum;
 	bool		isNull;
 
 	Assert(earm != NULL);
 	Assert(epqstate->origslot != NULL);
 
+	erm = earm->rowmark;
+
 	if (RowMarkRequiresRowShareLock(erm->markType))
 		elog(ERROR, "EvalPlanQual doesn't support locking rowmarks");
 
#2Daniel Gustafsson
daniel@yesql.se
In reply to: Dmitry Koval (#1)
Re: Assignment before assert

On 13 Feb 2025, at 18:08, Dmitry Koval <d.koval@postgrespro.ru> wrote:

Hi!
Function EvalPlanQualFetchRowMark contains an assignment

ExecRowMark *erm = earm->rowmark;

before assert

Assert(earm != NULL);

Maybe these lines need to be swapped?

That does admittedly look a bit odd, that assertion can't be reached if earm is
null since we've already dereferenced it by then. I'll have another look after
coffee but something along the lines of your patch looks right (or just remove
the Assertion perhaps).

--
Daniel Gustafsson

#3Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#2)
Re: Assignment before assert

On Fri, Feb 14, 2025 at 12:15:40AM +0100, Daniel Gustafsson wrote:

On 13 Feb 2025, at 18:08, Dmitry Koval <d.koval@postgrespro.ru> wrote:

Hi!
Function EvalPlanQualFetchRowMark contains an assignment

ExecRowMark *erm = earm->rowmark;

before assert

Assert(earm != NULL);

Maybe these lines need to be swapped?

That does admittedly look a bit odd, that assertion can't be reached if earm is
null since we've already dereferenced it by then. I'll have another look after
coffee but something along the lines of your patch looks right (or just remove
the Assertion perhaps).

Accessing earm->rowmark is one thing, assuming that earm should never
be NULL is a different one. Looking at 27cc7cd2bc8a that has
introduced this assertion, the proposed patch makes sense to me.
--
Michael

#4Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#3)
Re: Assignment before assert

On 14 Feb 2025, at 06:44, Michael Paquier <michael@paquier.xyz> wrote:

the proposed patch makes sense to me.

Committed, with a backpatch down to 13.

--
Daniel Gustafsson