Thread: Re: [COMMITTERS] Re: pgsql: Convert contrib/seg's bool-returning SQL functions to V1 call co

On Tue, Apr 26, 2016 at 10:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> I don't understand why we don't just drop V0. It makes debugging harder,
>> exploitation easier (call arbitrary functions), and really has no
>> features making it desirable.
>
> What's the argument that it makes debugging harder?  Especially if
> you aren't using it?
>
> I don't particularly buy the "easier exploitation" argument, either.
> You can't create a C function without superuser, and if you've got
> superuser there are plenty of ways to run arbitrary code.
>
> I'd agree that there are no desirable features that would motivate
> writing new code in V0.  But that's not the reason for keeping it;
> the reason for keeping it is to avoid unnecessarily breaking
> existing extension code.

I don't think that argument holds much water any more.  The V1
interface was added in 0a7fb4e9184539afcb6fed0f1d2bc0abddc2b0a6, more
than 15 years ago.  Anybody who has extension code that old that still
does anything useful and hasn't needed much bigger changes that
conversion to V1 calling convention deserves a medal.  But more than
that, it's unreasonable to expect 15-year-plus deprecation windows for
features that are only exposed via C.  If we stuck to that rigidly it
would be a major impediment to forward progress.

Let's add a GUC allow_oldstyle_functions with a default of off, and
make fetch_finfo_record() throw an ERROR in the infofunc == NULL case
unless allow_oldstyle_functions = on.  That way, anybody who still
really wants to use V0 can do so.  For everybody else, the very first
attempt to execute a function where you've forgotten or fat-fingered
the PG_FUNCTION_INFO_V1 decoration will produce a clear error message
telling you exactly what you did wrong.  I confidently predict a lot
more people will be happy about that development than will be sad
about having to set a GUC to make their V0 functions work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> Let's add a GUC allow_oldstyle_functions with a default of off, and
> make fetch_finfo_record() throw an ERROR in the infofunc == NULL case
> unless allow_oldstyle_functions = on.

This is not about "V0 calling convention is awful".  It isn't, really,
unless you need to deal with nulls.  This is about "allowing the finfo
record to be missing is error-prone".  What about inventing a
PG_FUNCTION_INFO_V0() macro, and then defining the new GUC as "must
find a matching finfo record"?  That would present less conversion
work for people who still have V0-style functions (and I'm sure there
are more than a few of them out there).

Now, if VS2015 actually has broken bool-returning V0, the argument for
keeping it going becomes a lot weaker.  But at this point I suspect
strongly that that's not what happened but rather we've got a faulty
bool cast there, which if true is something we need to fix regardless
of any considerations about V0.  Would someone please try the experiment
requested upthread?
        regards, tom lane

On Wed, Apr 27, 2016 at 9:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Let's add a GUC allow_oldstyle_functions with a default of off, and
>> make fetch_finfo_record() throw an ERROR in the infofunc == NULL case
>> unless allow_oldstyle_functions = on.
>
> This is not about "V0 calling convention is awful".  It isn't, really,
> unless you need to deal with nulls.  This is about "allowing the finfo
> record to be missing is error-prone".  What about inventing a
> PG_FUNCTION_INFO_V0() macro, and then defining the new GUC as "must
> find a matching finfo record"?  That would present less conversion
> work for people who still have V0-style functions (and I'm sure there
> are more than a few of them out there).

I'm not, but I don't have a problem with PG_FUNCTION_INFO_V0.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Apr 27, 2016 at 10:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Now, if VS2015 actually has broken bool-returning V0, the argument for
> keeping it going becomes a lot weaker.  But at this point I suspect
> strongly that that's not what happened but rather we've got a faulty
> bool cast there, which if true is something we need to fix regardless
> of any considerations about V0.  Would someone please try the experiment
> requested upthread?

I just gave it a try. And by using PG_1_BYTE() the tests of
contrib/seg/ are passing.
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Wed, Apr 27, 2016 at 10:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Now, if VS2015 actually has broken bool-returning V0, the argument for
>> keeping it going becomes a lot weaker.  But at this point I suspect
>> strongly that that's not what happened but rather we've got a faulty
>> bool cast there, which if true is something we need to fix regardless
>> of any considerations about V0.  Would someone please try the experiment
>> requested upthread?

> I just gave it a try. And by using PG_1_BYTE() the tests of
> contrib/seg/ are passing.

Thanks!  Barring objections, I will revert c8e81afc60093b19 tomorrow
and instead fix DatumGetBool.
        regards, tom lane