Обсуждение: Rationalizing code-sharing among src/bin/ directories

I'm not terribly happy with the hack I used to get pgbench to be able
to borrow psql's psqlscan.l lexer.  It's a mess for the MSVC build
scripts, and I have seen it causing two concurrent builds of psqlscan.o
in parallel builds, which is likely to cause a build failure some of
the time.  Another unresolved issue is that we can't apply FLEX_NO_BACKUP
testing to both of psql's lexers, because that causes each of those
make steps to want to write lex.backup in the current directory.

I have a modest proposal for improving this: let's move all the code
that's currently shared by two or more src/bin/ subdirectories into a
new directory, say src/feutils, and build it into a ".a" library in
the same way that src/common/ is handled.  This would remove the
following klugy cross-directory links:

src/bin/pgbench/psqlscan.o -> ../../../src/bin/psql/psqlscan.o
src/bin/psql/dumputils.c -> ../../../src/bin/pg_dump/dumputils.c
src/bin/psql/keywords.c -> ../../../src/bin/pg_dump/keywords.c
src/bin/scripts/dumputils.c -> ../../../src/bin/pg_dump/dumputils.c
src/bin/scripts/keywords.c -> ../../../src/bin/pg_dump/keywords.c
src/bin/scripts/mbprint.c -> ../../../src/bin/psql/mbprint.c
src/bin/scripts/print.c -> ../../../src/bin/psql/print.c

Note: the reason for a new subdirectory, rather than putting this
stuff into src/common/, is that src/common/ is meant for code that's
shared between frontend and backend, which this stuff is not.  Also,
many of these files depend on libpq which seems like an inappropriate
dependency for src/common.

Having said that, I also notice these dependencies:

src/bin/pg_dump/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
src/bin/psql/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
src/bin/scripts/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
src/interfaces/ecpg/preproc/kwlookup.c -> ../../../../src/backend/parser/kwlookup.c
src/bin/initdb/encnames.c -> ../../../src/backend/utils/mb/encnames.c
src/interfaces/libpq/encnames.c -> ../../../src/backend/utils/mb/encnames.c

which seem like they'd be better handled by moving those files into
src/common/.

Thoughts?
        regards, tom lane

On Wed, Mar 23, 2016 at 12:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm not terribly happy with the hack I used to get pgbench to be able
> to borrow psql's psqlscan.l lexer.  It's a mess for the MSVC build
> scripts, and I have seen it causing two concurrent builds of psqlscan.o
> in parallel builds, which is likely to cause a build failure some of
> the time.  Another unresolved issue is that we can't apply FLEX_NO_BACKUP
> testing to both of psql's lexers, because that causes each of those
> make steps to want to write lex.backup in the current directory.
>
> I have a modest proposal for improving this: let's move all the code
> that's currently shared by two or more src/bin/ subdirectories into a
> new directory, say src/feutils, and build it into a ".a" library in
> the same way that src/common/ is handled.  This would remove the
> following klugy cross-directory links:
>
> src/bin/pgbench/psqlscan.o -> ../../../src/bin/psql/psqlscan.o
> src/bin/psql/dumputils.c -> ../../../src/bin/pg_dump/dumputils.c
> src/bin/psql/keywords.c -> ../../../src/bin/pg_dump/keywords.c
> src/bin/scripts/dumputils.c -> ../../../src/bin/pg_dump/dumputils.c
> src/bin/scripts/keywords.c -> ../../../src/bin/pg_dump/keywords.c
> src/bin/scripts/mbprint.c -> ../../../src/bin/psql/mbprint.c
> src/bin/scripts/print.c -> ../../../src/bin/psql/print.c
>
> Note: the reason for a new subdirectory, rather than putting this
> stuff into src/common/, is that src/common/ is meant for code that's
> shared between frontend and backend, which this stuff is not.  Also,
> many of these files depend on libpq which seems like an inappropriate
> dependency for src/common.
>
> Having said that, I also notice these dependencies:
>
> src/bin/pg_dump/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
> src/bin/psql/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
> src/bin/scripts/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
> src/interfaces/ecpg/preproc/kwlookup.c -> ../../../../src/backend/parser/kwlookup.c
> src/bin/initdb/encnames.c -> ../../../src/backend/utils/mb/encnames.c
> src/interfaces/libpq/encnames.c -> ../../../src/backend/utils/mb/encnames.c
>
> which seem like they'd be better handled by moving those files into
> src/common/.
>
> Thoughts?

No objection.

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

On 23/03/16 17:55, Tom Lane wrote:
> I'm not terribly happy with the hack I used to get pgbench to be able
> to borrow psql's psqlscan.l lexer.  It's a mess for the MSVC build
> scripts, and I have seen it causing two concurrent builds of psqlscan.o
> in parallel builds, which is likely to cause a build failure some of
> the time.  Another unresolved issue is that we can't apply FLEX_NO_BACKUP
> testing to both of psql's lexers, because that causes each of those
> make steps to want to write lex.backup in the current directory.
>
> I have a modest proposal for improving this: let's move all the code
> that's currently shared by two or more src/bin/ subdirectories into a
> new directory, say src/feutils, and build it into a ".a" library in
> the same way that src/common/ is handled.  This would remove the
> following klugy cross-directory links:
>
> src/bin/pgbench/psqlscan.o -> ../../../src/bin/psql/psqlscan.o
> src/bin/psql/dumputils.c -> ../../../src/bin/pg_dump/dumputils.c
> src/bin/psql/keywords.c -> ../../../src/bin/pg_dump/keywords.c
> src/bin/scripts/dumputils.c -> ../../../src/bin/pg_dump/dumputils.c
> src/bin/scripts/keywords.c -> ../../../src/bin/pg_dump/keywords.c
> src/bin/scripts/mbprint.c -> ../../../src/bin/psql/mbprint.c
> src/bin/scripts/print.c -> ../../../src/bin/psql/print.c
>
> Note: the reason for a new subdirectory, rather than putting this
> stuff into src/common/, is that src/common/ is meant for code that's
> shared between frontend and backend, which this stuff is not.  Also,
> many of these files depend on libpq which seems like an inappropriate
> dependency for src/common.
>
> Having said that, I also notice these dependencies:
>
> src/bin/pg_dump/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
> src/bin/psql/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
> src/bin/scripts/kwlookup.c -> ../../../src/backend/parser/kwlookup.c
> src/interfaces/ecpg/preproc/kwlookup.c -> ../../../../src/backend/parser/kwlookup.c
> src/bin/initdb/encnames.c -> ../../../src/backend/utils/mb/encnames.c
> src/interfaces/libpq/encnames.c -> ../../../src/backend/utils/mb/encnames.c
>
> which seem like they'd be better handled by moving those files into
> src/common/.
>
> Thoughts?
>

Yes please!

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

Tom Lane wrote:

> Note: the reason for a new subdirectory, rather than putting this
> stuff into src/common/, is that src/common/ is meant for code that's
> shared between frontend and backend, which this stuff is not.  Also,
> many of these files depend on libpq which seems like an inappropriate
> dependency for src/common.

Actually you could just list them in OBJS_FRONTEND in src/common.  That
way they're not built for the server at all; no need for a new subdir.
The libpq dependency argument AFAICS only applies to the ones in
src/bin/psql.  Perhaps we could have feutils with those only, and move
the other files to src/common.

I'm unclear on the #ifndef PGSCRIPTS thingy in mbprint.c.  Perhaps it's
no longer needed?

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Note: the reason for a new subdirectory, rather than putting this
>> stuff into src/common/, is that src/common/ is meant for code that's
>> shared between frontend and backend, which this stuff is not.  Also,
>> many of these files depend on libpq which seems like an inappropriate
>> dependency for src/common.

> Actually you could just list them in OBJS_FRONTEND in src/common.  That
> way they're not built for the server at all; no need for a new subdir.

Meh.  I think stuff in OBJS_FRONTEND has to be pretty weird special
cases, such as frontend emulations of server-environment code.  Which
is what fe_memutils is, so that's OK, but I kinda question whether
restricted_token.c belongs in src/common/ at all.

> The libpq dependency argument AFAICS only applies to the ones in
> src/bin/psql.  Perhaps we could have feutils with those only, and move
> the other files to src/common.

On looking closer, the only one that doesn't depend on libpq is
keywords.c, which seems like it belongs in the same place as kwlookup.c.
So yeah, let's move both of those to src/common.

Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
would be more readable.  And where to put the corresponding header files?
src/include/fe-utils?
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:

> > Actually you could just list them in OBJS_FRONTEND in src/common.  That
> > way they're not built for the server at all; no need for a new subdir.
> 
> Meh.  I think stuff in OBJS_FRONTEND has to be pretty weird special
> cases, such as frontend emulations of server-environment code.  Which
> is what fe_memutils is, so that's OK, but I kinda question whether
> restricted_token.c belongs in src/common/ at all.

OK, that makes sense.  Feel free to move restricted_token.c if you feel
like it.

> > The libpq dependency argument AFAICS only applies to the ones in
> > src/bin/psql.  Perhaps we could have feutils with those only, and move
> > the other files to src/common.
> 
> On looking closer, the only one that doesn't depend on libpq is
> keywords.c, which seems like it belongs in the same place as kwlookup.c.
> So yeah, let's move both of those to src/common.

I guess those dependencies are somewhat hidden.  No objections to that
move.

> Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
> would be more readable.

Yes, +1 for either a - or _ in there.

> And where to put the corresponding header files?
> src/include/fe-utils?

Sounds fair but would that be installed in PREFIX/include/server?
That'd be a bit odd, but I'm not -1 on that.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> And where to put the corresponding header files?
>> src/include/fe-utils?

> Sounds fair but would that be installed in PREFIX/include/server?
> That'd be a bit odd, but I'm not -1 on that.

The only other plan I can think of is to put the .h files describing
src/fe-utils files into src/fe-utils itself.  Which would sort of match
the existing precedent of src/bin/ files looking into sibling directories
for relevant .h files, but it seems a bit odd too.
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> And where to put the corresponding header files?
> >> src/include/fe-utils?
> 
> > Sounds fair but would that be installed in PREFIX/include/server?
> > That'd be a bit odd, but I'm not -1 on that.
> 
> The only other plan I can think of is to put the .h files describing
> src/fe-utils files into src/fe-utils itself.  Which would sort of match
> the existing precedent of src/bin/ files looking into sibling directories
> for relevant .h files, but it seems a bit odd too.

I think it's better that they end up installed in
PREFIX/include/server/fe_utils rather than not installed at all, which
is what would happen, unless you were to create a tailored command in
make install.  For instance we have the ones for src/common in
PREFIX/include/server/common, which I suppose is useful for extensions
(we do install libpgcommon.a, which would be messy to use otherwise.)

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

I wrote:
> Having said that, I also notice these dependencies:
> ...
> src/bin/initdb/encnames.c -> ../../../src/backend/utils/mb/encnames.c
> src/interfaces/libpq/encnames.c -> ../../../src/backend/utils/mb/encnames.c
> ...
> which seem like they'd be better handled by moving those files into
> src/common/.

After poking around a bit, it seems like it ought to be a win to move
both encnames.c and wchar.c into src/common/, as both of those modules
are meant to be built for both backend and frontend environments.  The
stumbling block is pg_wchar.h, which is a total fail modularity-wise,
as it does not bother to distinguish stuff which should be visible to the
general backend from stuff which should be visible to frontend programs
from stuff which is solely of interest to encoding conversion logic (and
in many cases only to *particular* conversions).  I think we should not do
this move without figuring out which parts of that file sanely belong in a
src/include/common/ header file and which don't.  That's not something
that I feel like tackling right now, as it's been in that same messy state
for awhile and recent changes haven't made it worse.  But it ought to be
revisited at some point.
        regards, tom lane

On Wed, Mar 23, 2016 at 4:00 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
>> Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
>> would be more readable.
>
> Yes, +1 for either a - or _ in there.

I vote for an underscore, since that's what we mostly do.

[rhaas pgsql]$ find . -type d | awk -F/ '{print $NF}' | grep - | wc -l      6
[rhaas pgsql]$ find . -type d | awk -F/ '{print $NF}' | grep _ | wc -l     74

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

I wrote:
> I have a modest proposal for improving this: let's move all the code
> that's currently shared by two or more src/bin/ subdirectories into a
> new directory, say src/feutils, and build it into a ".a" library in
> the same way that src/common/ is handled.

Moving along on this project: I'm looking at pg_dump's dumputils.c/.h,
which contains a fair amount of generally-useful stuff but also some
stuff that seems to have no earthly use outside pg_dump/pg_dumpall.
In particular I do not see the point of moving these things to a shared
directory:

PGDUMP_STRFTIME_FMT macro

extern bool buildACLCommands(const char *name, const char *subname,                const char *type, const char *acls,
constchar *owner,                const char *prefix, int remoteVersion,                PQExpBuffer sql);
 
extern bool buildDefaultACLCommands(const char *type, const char *nspname,                       const char *acls,
constchar *owner,                       int remoteVersion,                       PQExpBuffer sql);
 
extern void buildShSecLabelQuery(PGconn *conn, const char *catalog_name,                    uint32 objectId,
PQExpBuffersql);
 
extern void emitShSecLabels(PGconn *conn, PGresult *res,               PQExpBuffer buffer, const char *target, const
char*objname);
 

What I propose doing is leaving the above-listed items in
pg_dump/dumputils.h/.c, and moving the rest of what's in those files
to new files src/include/fe_utils/string_utils.h and
src/fe_utils/string_utils.c.  This name is a bit arbitrary, but most of
what's there is string processing of some flavor or other, with some list
processing thrown in for good measure.  If anyone's got a different color
to paint this bikeshed, please speak up.
        regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 23, 2016 at 4:00 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Yes, +1 for either a - or _ in there.

> I vote for an underscore, since that's what we mostly do.

Yup, I had just counted and come to the same conclusion.  fe_utils/
it shall be.
        regards, tom lane

Tom Lane wrote:

> What I propose doing is leaving the above-listed items in
> pg_dump/dumputils.h/.c, and moving the rest of what's in those files
> to new files src/include/fe_utils/string_utils.h and
> src/fe_utils/string_utils.c.

Seems reasonable.

> This name is a bit arbitrary, but most of what's there is string
> processing of some flavor or other, with some list processing thrown
> in for good measure.  If anyone's got a different color to paint this
> bikeshed, please speak up.

I wondered about the list stuff while messing about in pg_dump awhile
ago.  It seems moderately okay, but not terribly generic; maybe we
should get rid of all that stuff and make ilist.c available to frontend.
Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
maybe we can discuss that in the future, to avoid blocking your patch.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> This name is a bit arbitrary, but most of what's there is string
>> processing of some flavor or other, with some list processing thrown
>> in for good measure.  If anyone's got a different color to paint this
>> bikeshed, please speak up.

> I wondered about the list stuff while messing about in pg_dump awhile
> ago.  It seems moderately okay, but not terribly generic; maybe we
> should get rid of all that stuff and make ilist.c available to frontend.
> Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
> maybe we can discuss that in the future, to avoid blocking your patch.

It wouldn't be terribly hard to split the file into, say, string_utils
and simple_list files.  On reflection that seems like a good idea as
long as we're going for cleanup rather than a one-to-one rename.
        regards, tom lane

Hi there,

> On 23 Mar 2016, at 22:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
> would be more readable.  And where to put the corresponding header files?
> src/include/fe-utils?


For me “utils" sounds like something of auxiliary nature. if some pretty
basic stuff is going to be added there, then I believe that fe_common or
perhaps fe_shared would be a bit more suitable.

Regards,
Aleksey

Aleksey Demakov <a.demakov@postgrespro.ru> writes:
> Hi there,
>> On 23 Mar 2016, at 22:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Anybody want to bikeshed the directory name src/feutils?  Maybe fe-utils
>> would be more readable.  And where to put the corresponding header files?
>> src/include/fe-utils?

> For me “utils" sounds like something of auxiliary nature. if some pretty
> basic stuff is going to be added there, then I believe that fe_common or
> perhaps fe_shared would be a bit more suitable.

Meh...  The stuff that is in-scope for it at the moment doesn't seem
all that basic.  It's just assorted widgets that turned out to be
useful outside their original home.  Really basic stuff is going into
src/common/ these days.
        regards, tom lane

On Fri, Mar 25, 2016 at 1:11 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Tom Lane wrote:
>
>> What I propose doing is leaving the above-listed items in
>> pg_dump/dumputils.h/.c, and moving the rest of what's in those files
>> to new files src/include/fe_utils/string_utils.h and
>> src/fe_utils/string_utils.c.
>
> Seems reasonable.
>
>> This name is a bit arbitrary, but most of what's there is string
>> processing of some flavor or other, with some list processing thrown
>> in for good measure.  If anyone's got a different color to paint this
>> bikeshed, please speak up.
>
> I wondered about the list stuff while messing about in pg_dump awhile
> ago.  It seems moderately okay, but not terribly generic; maybe we
> should get rid of all that stuff and make ilist.c available to frontend.
> Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
> maybe we can discuss that in the future, to avoid blocking your patch.

Definitely worth it, list mimics of what is in pg_dump are located in
pg_basebackup, and since a 9.6 patch in psql as well. That's as much
code duplication.

Preventing the use of elog in the frontend is something that has been
addressed multiple times with FRONTEND, so that's not likely going to
be an issue I think. Andres has mentioned as well having some elog
stuff available in frontend..
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Mar 25, 2016 at 1:11 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> I wondered about the list stuff while messing about in pg_dump awhile
>> ago.  It seems moderately okay, but not terribly generic; maybe we
>> should get rid of all that stuff and make ilist.c available to frontend.
>> Not sure how easy is that, given that AFAIR ilist uses elog.  Anyway
>> maybe we can discuss that in the future, to avoid blocking your patch.

> Definitely worth it, list mimics of what is in pg_dump are located in
> pg_basebackup, and since a 9.6 patch in psql as well. That's as much
> code duplication.

Well, it's not a *lot* of code duplication.  Agreed, it's a bit ugly
that we've got three or four copied-and-pasted versions of
simple_foo_list_append, but I'm not convinced that it'd be worth the
trouble to do anything about that.  If FE code starts needing more list
functionality than that, maybe importing ilist or similar would be
worthwhile, but right now I think it would be overkill.

> Preventing the use of elog in the frontend is something that has been
> addressed multiple times with FRONTEND, so that's not likely going to
> be an issue I think. Andres has mentioned as well having some elog
> stuff available in frontend..

I'm on board with doing something about that one, though.  There are
quite a lot of places that could be cleaned up.
        regards, tom lane

On Fri, Mar 25, 2016 at 9:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> Preventing the use of elog in the frontend is something that has been
>> addressed multiple times with FRONTEND, so that's not likely going to
>> be an issue I think. Andres has mentioned as well having some elog
>> stuff available in frontend..
>
> I'm on board with doing something about that one, though.  There are
> quite a lot of places that could be cleaned up.

Not sure if Andres is working on that for now or not, the main
discussion that I am foreseeing here is how we are going to map elevel
for the frontend (should FATAL, PANIC exit immediately, etc). I think
as well that some sort of callback system would be needed to allow
frontend clients to perform filtering of the log entries. Say
pg_rewind has a --debug mode, so this would map with DEBUG1 entries,
we'd need a way to control if those are issued or not depending on the
frontend call arguments...

Surely that's a cleanup patch 9.7 though, so there are a couple of
months ahead of us... And that's not the highest priority now.
Stability of 9.6 is first and just ahead.
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> Not sure if Andres is working on that for now or not, the main
> discussion that I am foreseeing here is how we are going to map elevel
> for the frontend (should FATAL, PANIC exit immediately, etc).

Doesn't seem that complicated to me: elevel >= ERROR results in exit(1),
otherwise just print to stderr and return.  We'd be eyeballing each case
that we remove "#ifndef FRONTEND" from anyway; if it's expecting behavior
noticeably more complicated than that, we could leave it as-is.

> Stability of 9.6 is first and just ahead.

Agreed, this is code cleanup not a high priority.
        regards, tom lane

On Fri, Mar 25, 2016 at 12:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> Not sure if Andres is working on that for now or not, the main
>> discussion that I am foreseeing here is how we are going to map elevel
>> for the frontend (should FATAL, PANIC exit immediately, etc).
>
> Doesn't seem that complicated to me: elevel >= ERROR results in exit(1),
> otherwise just print to stderr and return.  We'd be eyeballing each case
> that we remove "#ifndef FRONTEND" from anyway; if it's expecting behavior
> noticeably more complicated than that, we could leave it as-is.

Something that I see as mandatory as well is a way to bypass some of
the elevels depending on the way a frontend tool is called, so we'd
need something like that in the common elog facility:

void
elog(blah)
{
#ifdef FRONTEND    if (!callback_routine_defined_in_frontend(elevel))        return;
#endif
    blah_blah_move_on.
}

This would be useful to avoid code patterns of this type in a frontend
tool where for example a debug flag is linked with elevel. For example
this pattern:
if (debug)   elog(DEBUG1, "Debug blah");
could be reduced to as the callback routine would allow bypassing
elog() directly:
elog(DEBUG1, "Debug blah");

That's just food for thought at this stage, I get back to reviewing...
-- 
Michael