Thread: Do we really need to switch to per-tuple memory context inATRewriteTable() when Table Rewrite is not happening

Hi All,

Today while trying to understand the code for ALTER TABLE in
PostgreSQL (basically the table rewrite part), I noticed that we are
switching to a per-tuple memory context even when table rewrite is not
required. For e.g.. consider the case where we do ADD CONSTRAINTS (NOT
NULL or CHECK) using ALTER TABLE command. In this case, we just scan
the tuple and verify it for the given constraint instead of forming a
new tuple. So, not sure why do we switch to per-tuple memory context
when just adding the constraint. Could someone please let me know the
reason for doing so. Thanks in advance.

I am basically talking about the following lines of code in
ATRewriteTable() function.

/*
 * Switch to per-tuple memory context and reset it for each tuple
 * produced, so we don't leak memory.
 */
oldCxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));


AFAICU, we should have done that only when 'tab->rewrite > 0' is true
or may be when 'OIDNewHeap' is valid.

Here are the steps that i have followed to understand ATRewriteTable(),

CREATE TABLE tmp (initial int4);

INSERT INTO tmp VALUES(10);
INSERT INTO tmp VALUES(20);

ALTER TABLE tmp ADD CONSTRAINT check_cons CHECK (initial > 5);
ALTER TABLE tmp ALTER COLUMN initial SET NOT NULL;

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Ashutosh Sharma wrote:

> I am basically talking about the following lines of code in
> ATRewriteTable() function.
> 
> /*
>  * Switch to per-tuple memory context and reset it for each tuple
>  * produced, so we don't leak memory.
>  */
> oldCxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));

Perhaps a memory context switch is so cheap that adding a branch to
verify whether it's needed is more expensive than just doing it all the
time.  You could prove me wrong by measuring it.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

>>>>> "Ashutosh" == Ashutosh Sharma <ashu.coek88@gmail.com> writes:

 Ashutosh> Hi All,

 Ashutosh> Today while trying to understand the code for ALTER TABLE in
 Ashutosh> PostgreSQL (basically the table rewrite part), I noticed that
 Ashutosh> we are switching to a per-tuple memory context even when
 Ashutosh> table rewrite is not required. For e.g.. consider the case
 Ashutosh> where we do ADD CONSTRAINTS (NOT NULL or CHECK) using ALTER
 Ashutosh> TABLE command. In this case, we just scan the tuple and
 Ashutosh> verify it for the given constraint instead of forming a new
 Ashutosh> tuple. So, not sure why do we switch to per-tuple memory
 Ashutosh> context when just adding the constraint.

What makes you think that evaluating the constraint won't allocate
memory?

Switching contexts is virtually free (just an assignment to a global
var). Resetting a context that's not been allocated in since its last
reset is also virtually free (just checks a flag). In contrast, every
single function (except special cases like index comparators) is free to
allocate memory in its caller's context, for temporary use and for
returning the result; how else would a condition like
CHECK(substring(col for 3)='FOO') work, without allocating memory for
the substring result?

-- 
Andrew (irc:RhodiumToad)

On Wed, Jan 3, 2018 at 7:25 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Ashutosh Sharma wrote:
>
>> I am basically talking about the following lines of code in
>> ATRewriteTable() function.
>>
>> /*
>>  * Switch to per-tuple memory context and reset it for each tuple
>>  * produced, so we don't leak memory.
>>  */
>> oldCxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
>
> Perhaps a memory context switch is so cheap that adding a branch to
> verify whether it's needed is more expensive than just doing it all the
> time.  You could prove me wrong by measuring it.
>

might be...I'm not sure about the cost of context switching. As you
said, it is quite possible that  adding a check to verify whether
switching is required or not could be more expensive than doing the
context switching itself.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

> --
> Álvaro Herrera                https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 3, 2018 at 8:06 PM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>>>>>> "Ashutosh" == Ashutosh Sharma <ashu.coek88@gmail.com> writes:
>
>  Ashutosh> Hi All,
>
>  Ashutosh> Today while trying to understand the code for ALTER TABLE in
>  Ashutosh> PostgreSQL (basically the table rewrite part), I noticed that
>  Ashutosh> we are switching to a per-tuple memory context even when
>  Ashutosh> table rewrite is not required. For e.g.. consider the case
>  Ashutosh> where we do ADD CONSTRAINTS (NOT NULL or CHECK) using ALTER
>  Ashutosh> TABLE command. In this case, we just scan the tuple and
>  Ashutosh> verify it for the given constraint instead of forming a new
>  Ashutosh> tuple. So, not sure why do we switch to per-tuple memory
>  Ashutosh> context when just adding the constraint.
>
> What makes you think that evaluating the constraint won't allocate
> memory?
>
> Switching contexts is virtually free (just an assignment to a global
> var). Resetting a context that's not been allocated in since its last
> reset is also virtually free (just checks a flag). In contrast, every
> single function (except special cases like index comparators) is free to
> allocate memory in its caller's context, for temporary use and for
> returning the result; how else would a condition like
> CHECK(substring(col for 3)='FOO') work, without allocating memory for
> the substring result?
>

I am not saying that we do not do memory allocation for constraints.
We do that in some cases as the one you mentioned above, but, for that
we are already doing context switching inside ExecCheck().

ExecCheck()-->
ExecEvalExprSwitchContext()->MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory)...state->evalfunc(state,
econtext, isNull) ...MemoryContextSwitchTo(oldContext);

So, my point is, why are we doing an extra context switching inside
ATRewriteTable() for constraints,

/*
 * Switch to per-tuple memory context and reset it for each tuple
 * produced, so we don't leak memory.
 */
oldCxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

> --
> Andrew (irc:RhodiumToad)