Why is there a doubtful copyObject call in add_vars_to_targetlist

(Redirecting to Hackers, since Novice is not the correct place for this
question)

On 13 March 2017 at 14:22, Neha Khatri <nehakhatri5@gmail.com> wrote:

Hi,

I was debugging that when does the function _copyVar get invoked, and the
first hit for that was in the add_vars_to_targetlist. There I happened to
see the following comment:

/* XXX is copyObject necessary here? */

Further digging showed that this copyObject got added in the commit
5efe3121:

+       /* XXX is copyObject necessary here? */
+ rel->targetlist = lappend(rel->targetlist,
+                           create_tl_element((Var *) copyObject(var),
+                                             length(rel->targetlist) +
1));

This copyObject still exits in the current code. So I was wondering if the
comment question still holds good and why the question there in first place.
To make a new Var object, copyObject seem to be the right choice, then why
the doubt?

The doubt is in the fact if copyObject() is required at all. The other
option being to simply reference the same object without having made a copy.

The danger of not making a copy would be that any changes made would
naturally affect all things which reference the object. It would seem the
comment and the copyObject() are still there because nobody is satisfied
that it's not required enough to go and remove it, that weighted against
the fact that removing likely wouldn't buy that much performance wise is
likely the reason it's still there.

Probably if someone came up with a realistic enough case to prove that it
was worth removing, then someone might take some time to go and check if it
was safe to remove. There's a good chance that it'll not happen until then,
giving that nobody's bothered in almost 18 years.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:

On 13 March 2017 at 14:22, Neha Khatri <nehakhatri5@gmail.com> wrote:

This copyObject still exits in the current code. So I was wondering if the
comment question still holds good and why the question there in first place.
To make a new Var object, copyObject seem to be the right choice, then why
the doubt?

The doubt is in the fact if copyObject() is required at all. The other
option being to simply reference the same object without having made a copy.

Right. Note that the code that 5efe3121 replaced effectively made a new
Var object using makeVar. The new code makes a new Var object using
copyObject, so there's no actual behavioral change in that fragment, just
fewer lines of code. But it's fair to wonder whether it wouldn't be safe
just to link to the existing Var object. This is tied up in the planner's
general willingness to scribble on its input data structures, so that
linking to a pre-existing object is vulnerable to some unrelated bit of
code deciding to scribble on that object. Ideally that wouldn't happen
... but cleaning it up looks like a mighty tedious bit of janitorial work,
with uncertain payoff. So it hasn't happened in the last twenty years
and I'm not prepared to bet that it ever will.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Mar 13, 2017 at 3:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <david.rowley@2ndquadrant.com> writes:

On 13 March 2017 at 14:22, Neha Khatri <nehakhatri5@gmail.com> wrote:

This copyObject still exits in the current code. So I was wondering if

the

comment question still holds good and why the question there in first

place.

To make a new Var object, copyObject seem to be the right choice, then

why

the doubt?

The doubt is in the fact if copyObject() is required at all. The other
option being to simply reference the same object without having made a

copy.

Right. Note that the code that 5efe3121 replaced effectively made a new
Var object using makeVar. The new code makes a new Var object using
copyObject, so there's no actual behavioral change in that fragment, just
fewer lines of code. But it's fair to wonder whether it wouldn't be safe
just to link to the existing Var object. This is tied up in the planner's
general willingness to scribble on its input data structures, so that
linking to a pre-existing object is vulnerable to some unrelated bit of
code deciding to scribble on that object. Ideally that wouldn't happen
... but cleaning it up looks like a mighty tedious bit of janitorial work,
with uncertain payoff. So it hasn't happened in the last twenty years
and I'm not prepared to bet that it ever will.

Then, should it be alright to remove the doubt itself?

Regards,
Neha

Neha Khatri <nehakhatri5@gmail.com> writes:

Then, should it be alright to remove the doubt itself?

It's a perfectly legitimate comment describing a potential optimization.
There are lots of other similar comments that might or might not ever get
addressed.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Sure, understood.

Regards,
Neha