Re: Commitfest patches mostly assigned ... status

Hackers,

At this point, almost all patches have been assigned to reviewers. If
you submitted a patch and don't get feedback by Saturday, take a look at
who's in the "reviewers" column and send them a query. Since I have no
way to track when patches are assigned to reviewers, I have no idea if
some of them are sitting on their hands.

Some patches have not been assigned to reviewers for various reasons.
The following weren't assigned because they are complex and really need
a high-end hacker or committer to take them on:

libpq events
rmgr hooks and contrib/rmgr_hook
CLUSTER using sort instead of index scan

The following were not assigned because there has already been
discussion on this list debating them being a good idea. These need
consensus on this list before they get assigned to a reviewer:

remove --inputdir and --outputdir from pg_regress
GUC: Case-insensitive units
Allow has_table_privilege(...,'usage') on sequences

I've assigned some reviewers to "WIP" patches with instructions to
report back on their general experience with building, functionality and
spec.

Note that patches need not have only one reviewer! If you have time,
please take on testing some of the more complex patches, especially:
Column-level Permissions
Common Table Expressions
SE-PostgreSQL patches

Also, note that the following patches need performance testing on a
variety of platforms. Everyone should help with this.
GSoC Improved Hash Indexing
posix fadvises
operator restrictivity function for text search
CLUSTER using sort instead of index scan

Thanks for you input, and let's close this commitfest in a week!

--CommitFest "Mom"

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Also, note that the following patches need performance testing on a
> variety of platforms. Everyone should help with this.
> GSoC Improved Hash Indexing
> posix fadvises
> operator restrictivity function for text search
> CLUSTER using sort instead of index scan

The last two of those are not code-complete so I'm not sure that
performance testing is all that meaningful for them. But by all
means, test the first two ...

regards, tom lane

On Thu, Sep 11, 2008 at 1:53 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Some patches have not been assigned to reviewers for various reasons. The
> following weren't assigned because they are complex and really need a
> high-end hacker or committer to take them on:
>
> libpq events

Alvaro actually performed a review on libpq events. We are waiting on
his feedback on our changes (based on his review) and the newly
submitted documentation. I'll update the wiki accordingly. I wasn't
sure if Alvaro was claiming reviewer status or not. It may be ready
to go in as-is unless we draw any new objections.

Anybody who wants to look should ping Alvaro and/or Tom.

merlin

Merlin Moncure escribió:
> On Thu, Sep 11, 2008 at 1:53 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> > Some patches have not been assigned to reviewers for various reasons. The
> > following weren't assigned because they are complex and really need a
> > high-end hacker or committer to take them on:
> >
> > libpq events
>
> Alvaro actually performed a review on libpq events. We are waiting on
> his feedback on our changes (based on his review) and the newly
> submitted documentation. I'll update the wiki accordingly. I wasn't
> sure if Alvaro was claiming reviewer status or not. It may be ready
> to go in as-is unless we draw any new objections.

The patch looks OK to me as it was the last time I looked at it; maybe
Tom has more comments, so I decided against just committing it.

I admit I haven't checked the docs.

Actually a minor gripe ... should PQsetvalue be PQsetValue? :-)

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
>
> Actually a minor gripe ... should PQsetvalue be PQsetValue? :-)
>

We were mimicing PQgetvalue, which uses a lowercase 'v'. Is there a
preference, none on this end.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Merlin Moncure escribi:
>> Alvaro actually performed a review on libpq events. We are waiting on
>> his feedback on our changes (based on his review) and the newly
>> submitted documentation. I'll update the wiki accordingly. I wasn't
>> sure if Alvaro was claiming reviewer status or not. It may be ready
>> to go in as-is unless we draw any new objections.

> The patch looks OK to me as it was the last time I looked at it; maybe
> Tom has more comments, so I decided against just committing it.

I haven't got round to looking at it in this fest. If anyone else wants
to look it over, feel free. I think Josh is overrating the patch's
review difficulty --- anyone who uses libpq a lot could review it,
IMHO. You certainly wouldn't need any backend-internals knowledge.

regards, tom lane

On Thu, Sep 11, 2008 at 5:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The patch looks OK to me as it was the last time I looked at it; maybe
>> Tom has more comments, so I decided against just committing it.
>
> I haven't got round to looking at it in this fest. If anyone else wants
> to look it over, feel free. I think Josh is overrating the patch's
> review difficulty --- anyone who uses libpq a lot could review it,
> IMHO. You certainly wouldn't need any backend-internals knowledge.
>

Josh is probably basing that on the long history of
discussion/revision cycles. There is very little change from the
design that was hammered out except for the late breaking tweak of how
the passthru pointer works. I think the subtext here is that we are
waiting on you to sign off on the patch (or not).

merlin

> Josh is probably basing that on the long history of
> discussion/revision cycles.

Yep, and that *I* don't understand what the patch does, so I'm not going to
turn a newbie reviewer loose on it.

--
--Josh

Josh Berkus
PostgreSQL
San Francisco

Josh Berkus wrote:
>> Josh is probably basing that on the long history of
>> discussion/revision cycles.
>
> Yep, and that *I* don't understand what the patch does, so I'm not going to
> turn a newbie reviewer loose on it.
>

Here is a quick overview, there are two parts to the patch:

1. event system
This allows one to register "PQregisterEventProc" a per-conn callback function
with libpq that gets called when particular events occur. Currently, the events
tell you when a conn or result is created, reset, destroyed and copied. It is
generic enough to add more events in the future.

By receiving events about libpq objects, you can properly allocate and free
userland memory (PQfinish, PQexec, PQclear, etc... trigger events) and associate
it with the libpq object: thus PQsetInstanceData(conn...) or
PQsetResultInstanceData(res....). This is basically adding members to the
opaque PGconn or PGresult during runtime. This "instance data" can be retreived
via PQinstanceData and PQresultInstanceData. To shine a different light on it,
apps normally wrap a PGconn or PGresult within their own structures, but now you
can wrap the app structures inside a PGconn or PGresult.

This may help, its the libpqtypes PGEventProc implementation.
http://libpqtypes.esilo.com/browse_source.html?file=events.c

Also check out the patches sgml docs if you get a chance.

2. result management
There are also four functions that provide more control over PGresult. If you
need to create a result from scratch, expands on the PQmakeEmptyPGResult idea.

PQcopyResult - copy a given source result, flags argument determines what
portions of the result are copied.

PQsetResultAttrs - sets the attributes of the reuslt (its columns).

PQsetvalue - sets a tuple value in a result

PQresultAlloc - thin wrapper around the internal pqResultAlloc. Uses the
result's block allocater, which allows PQclear to properly free all memory
assocaited with a PGresult.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Josh Berkus wrote:
> Hackers,
>
> At this point, almost all patches have been assigned to reviewers. If
> you submitted a patch and don't get feedback by Saturday, take a look at
> who's in the "reviewers" column and send them a query. Since I have no
> way to track when patches are assigned to reviewers, I have no idea if
> some of them are sitting on their hands.
:
> Note that patches need not have only one reviewer! If you have time,
> please take on testing some of the more complex patches, especially:
> Column-level Permissions
> Common Table Expressions
> SE-PostgreSQL patches

I updated my patches:
http://archives.postgresql.org/pgsql-hackers/2008-09/msg00859.php

It contains rebasing to the latest CVS HEAD, a new hook to avoid
bypass access controls, and a small fix.

In addition, I tried to attach several short explanations about key points
to understand the big patch. I wrote the documentation of SE-PostgreSQL,
but it is not a description for *the patch*.
If reviewers have any unclear things, please feel free to ask me.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>