Re: Deprecation

Folks,

We have some really silly legacy stuff in PostgreSQL, the silliest of
which, as far as I've found, is the add_missing_from GUC.

Since it's been deprecated, as in off by default, since 8.1, I'd like
to suggest that we remove it from both docs and code and throw an
error if someone tries to set it, just as if they'd set
add_flying_spaghetti_monster.

What say?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david(at)fetter(dot)org> writes:
> We have some really silly legacy stuff in PostgreSQL, the silliest of
> which, as far as I've found, is the add_missing_from GUC.

Considering that we just had a discussion about a significant
application that's still using it, I'm not sure what's your hurry.
Is your intent specifically to break OpenACS in hopes of getting
their attention? If so, you need to be a bit more up-front about that.

(I would actually not mind getting rid of it, because that would greatly
simplify a problem I'm wrestling with right now, namely how to put hooks
into the parser for resolution of plpgsql variables. But we need to be
honest about what it's going to do to users.)

regards, tom lane

On Fri, Oct 16, 2009 at 01:23:16PM -0400, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> writes:
> > We have some really silly legacy stuff in PostgreSQL, the silliest
> > of which, as far as I've found, is the add_missing_from GUC.
>
> Considering that we just had a discussion about a significant
> application that's still using it, I'm not sure what's your hurry.
> Is your intent specifically to break OpenACS in hopes of getting
> their attention? If so, you need to be a bit more up-front about
> that.
>
> (I would actually not mind getting rid of it, because that would
> greatly simplify a problem I'm wrestling with right now, namely how
> to put hooks into the parser for resolution of plpgsql variables.
> But we need to be honest about what it's going to do to users.)

Breaking legacy applications is a side effect, one we should probably
publish far and wide, of the code cleanup.

My "hidden agenda," such as it is, is to make sure people don't get
the misapprehension that they can just "fire and forget" with
PostgreSQL, or any other software. Interfaces change; ugly kludges
get removed, and they need to build their processes around this fact
rather than around wishful thinking.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, Oct 16, 2009 at 10:23 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> (I would actually not mind getting rid of it, because that would greatly
> simplify a problem I'm wrestling with right now, namely how to put hooks
> into the parser for resolution of plpgsql variables.  But we need to be
> honest about what it's going to do to users.)

I was actually expecting you to come down on the side of "keep it
until it gets in the way". But if it's getting in the way already then
it seems reasonable to start talking about getting rid of it.

It only affects OpenACS if they want to upgrade to 8.5 which will
presumably mean other application changes as well. Though probaby none
requiring as much major code changes as this.

--
greg

Greg Stark <gsstark(at)mit(dot)edu> writes:
> On Fri, Oct 16, 2009 at 10:23 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> (I would actually not mind getting rid of it, because that would greatly
>> simplify a problem I'm wrestling with right now, namely how to put hooks
>> into the parser for resolution of plpgsql variables. But we need to be
>> honest about what it's going to do to users.)

> I was actually expecting you to come down on the side of "keep it
> until it gets in the way". But if it's getting in the way already then
> it seems reasonable to start talking about getting rid of it.

Yeah. The problem is that I'd like to have plpgsql install a hook that
runs at the end of transformColumnRef and has an opportunity to provide
a reference to a plpgsql var if nothing was found in the normal SQL
lookups. However, if the columnref looks like "x.y" where x happens to
match some table in the database (and not in the query) that doesn't
have a column y, the implicit-RTE code will have already modified the
querytree before finding out that column y doesn't exist. While that
can probably be unwound somehow, it's going to be a major PITA, and
there would be no way to hide the fact that a lock got taken on x before
we changed our minds about including it in the query. Considering that
this is all legacy behavior anyway it would be nice to not have to fix
it.

So, while I do think it's something we should leave alone until it gets
in the way, this is a sufficiently large value of "in the way" that I'm
willing to talk about removing add_missing_from. I'm just concerned
about the impact of that, considering that an app that still depends on
it came up as recently as yesterday.

regards, tom lane

On Fri, Oct 16, 2009 at 9:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So, while I do think it's something we should leave alone until it gets
> in the way, this is a sufficiently large value of "in the way" that I'm
> willing to talk about removing add_missing_from.  I'm just concerned
> about the impact of that, considering that an app that still depends on
> it came up as recently as yesterday.

8.4 is going to be supported for several years so they can document
it's the last version supported by the current version of OpenACS.

If they want the benefits from 8.5 and higher (for themselves or their
users), they'll do the work needed to remove the various bad practices
from their code.

Note that the removal of the implicit casts to text was far more
problematic for a lot of apps than add_missing_from=off - which is the
de facto standard for several years now.

And as you can see in
http://openacs.org/forums/message-view?message_id=2471518 , they did
the ground work to support the tsearch2 changes from contrib to core.
A friendly email to them explaining why it needs to be fixed in their
code should be sufficient (and it might be already fixed, btw).

--
Guillaume

On Fri, Oct 16, 2009 at 12:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> However, if the columnref looks like "x.y" where x happens to
> match some table in the database (and not in the query) that doesn't
> have a column y, the implicit-RTE code will have already modified the
> querytree before finding out that column y doesn't exist.

Hm, so if you do nothing then really the only thing that doesn't work
is if you have add_missing_from then plpgsql record variables wouldn't
work when you tried to reference their columns?

Perhaps we could just document that add_missing_from breaks that case?
The worst thing about that is that someone could switch that option on
globally on their server and break code that's part of some
third-party module.

--
greg

On Fri, 16 Oct 2009, Greg Stark wrote:

> It only affects OpenACS if they want to upgrade to 8.5 which will
> presumably mean other application changes as well. Though probaby none
> requiring as much major code changes as this.

Being one that hosts alot of OACS sites, and has a fair experience with
it, I agree ... as long as this isn't something that is going to be
backpatched, there should be no reason why this can't go in for 8.5 ...
the OACS guys will either fix the code, or stick to 8.4 ...

----
Marc G. Fournier Hub.Org Hosting Solutions S.A.
scrappy(at)hub(dot)org http://www.hub.org

Yahoo:yscrappy Skype: hub.org ICQ:7615664 MSN:scrappy(at)hub(dot)org

On Fri, 16 Oct 2009, Tom Lane wrote:

> So, while I do think it's something we should leave alone until it gets
> in the way, this is a sufficiently large value of "in the way" that I'm
> willing to talk about removing add_missing_from. I'm just concerned
> about the impact of that, considering that an app that still depends on
> it came up as recently as yesterday.

As this should / would only affect 8.5+, just means that the app in
question has to be stuck at 8.4 or fix the code. Since, as David points
out, this 'hack' has been in there since 8.1, I think we've given the app
in question more then sufficient time to fix their code already, no? 3
years, or so?

----
Marc G. Fournier Hub.Org Hosting Solutions S.A.
scrappy(at)hub(dot)org http://www.hub.org

Yahoo:yscrappy Skype: hub.org ICQ:7615664 MSN:scrappy(at)hub(dot)org

Greg Stark <gsstark(at)mit(dot)edu> writes:
> On Fri, Oct 16, 2009 at 12:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> However, if the columnref looks like "x.y" where x happens to
>> match some table in the database (and not in the query) that doesn't
>> have a column y, the implicit-RTE code will have already modified the
>> querytree before finding out that column y doesn't exist.

> Hm, so if you do nothing then really the only thing that doesn't work
> is if you have add_missing_from then plpgsql record variables wouldn't
> work when you tried to reference their columns?

"Do nothing" isn't the right phrase here --- it would take a great deal
of work and ugly, hard-to-maintain code to get it to work even that badly.
The code paths in transformColumnRef are fairly messy already :-(.
Getting rid of add_missing_from would definitely make it easier to
refactor to support hooks for external variable sources.

The approach I had been thinking about proposing, before David piped up
with his modest proposal, was to have external variables take precedence
over implicit RTEs --- ie, we'd call the hook *before* trying the
add_missing_from case. But that seems pretty weird, and it'd still be
messy to program. What it would mainly accomplish is to avoid the extra
lock hazard.

regards, tom lane

Tom Lane wrote:
> Greg Stark <gsstark(at)mit(dot)edu> writes:
> > On Fri, Oct 16, 2009 at 12:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> However, if the columnref looks like "x.y" where x happens to
> >> match some table in the database (and not in the query) that doesn't
> >> have a column y, the implicit-RTE code will have already modified the
> >> querytree before finding out that column y doesn't exist.
>
> > Hm, so if you do nothing then really the only thing that doesn't work
> > is if you have add_missing_from then plpgsql record variables wouldn't
> > work when you tried to reference their columns?
>
> "Do nothing" isn't the right phrase here --- it would take a great deal
> of work and ugly, hard-to-maintain code to get it to work even that badly.
> The code paths in transformColumnRef are fairly messy already :-(.
> Getting rid of add_missing_from would definitely make it easier to
> refactor to support hooks for external variable sources.
>
> The approach I had been thinking about proposing, before David piped up
> with his modest proposal, was to have external variables take precedence
> over implicit RTEs --- ie, we'd call the hook *before* trying the
> add_missing_from case. But that seems pretty weird, and it'd still be
> messy to program. What it would mainly accomplish is to avoid the extra
> lock hazard.

Sounds like a good reason to remove add_missing_from in 8.5.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> "Do nothing" isn't the right phrase here --- it would take a great deal
> of work and ugly, hard-to-maintain code to get it to work even that badly.
> The code paths in transformColumnRef are fairly messy already :-(.
> Getting rid of add_missing_from would definitely make it easier to
> refactor to support hooks for external variable sources.

A little voice from the field, +1 for deprecating add_missing_from.

Regards,
--
dim

2009/10/17 Dimitri Fontaine <dfontaine(at)hi-media(dot)com>:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> "Do nothing" isn't the right phrase here --- it would take a great deal
>> of work and ugly, hard-to-maintain code to get it to work even that badly.
>> The code paths in transformColumnRef are fairly messy already :-(.
>> Getting rid of add_missing_from would definitely make it easier to
>> refactor to support hooks for external variable sources.
>
> A little voice from the field, +1 for deprecating add_missing_from.
>

+1
Pavel

> Regards,
> --
> dim
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Sounds like a good reason to remove add_missing_from in 8.5.

Seems like the general consensus is that it's okay to do that.
I will go make it happen unless somebody squawks pretty soon...

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>
>> Sounds like a good reason to remove add_missing_from in 8.5.
>>
>
> Seems like the general consensus is that it's okay to do that.
> I will go make it happen unless somebody squawks pretty soon...
>

Squawk. I am currently travelling. Please give me until early next week
to research and react.

thanks

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Squawk. I am currently travelling. Please give me until early next week
> to research and react.

Okay, I'll hold off for a bit. For reference, attached is the patch
I was about to apply. This doesn't do any of the refactoring I had
in mind, it just removes the implicit-RTE logic.

regards, tom lane

On Sat, Oct 17, 2009 at 03:01:27PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Sounds like a good reason to remove add_missing_from in 8.5.
>
> Seems like the general consensus is that it's okay to do that.
> I will go make it happen unless somebody squawks pretty soon...
>
> regards, tom lane

+1

-dg

--
David Gould daveg(at)sonic(dot)net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.