Horton

Thread

Commits

  1. Fix inclusions of c.h from .h files.

  2. Fix inclusions of postgres_fe.h from .h files.

  3. Bring plpgsql into line with header inclusion policy.

  4. Document intentional violations of header inclusion policy.

  5. Remove inclusion of postgres.h from a few header files.

  1. Removing #include "postgres.h" from a couple of headers

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-03-08T08:55:08Z

    Hi,

Over in another thread it was pointed out that a patch I submitted
broke a project rule by including "postgres.h" in a header.  Here is a
patch to remove it from dsa.h where I made the same mistake, and also
a case I found in bufmask.h by grepping.

There are also instances in regcustom.h and snowball's header.h -- are
those special cases?

-- 
Thomas Munro
http://www.enterprisedb.com

  2. Re: Removing #include "postgres.h" from a couple of headers

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-08T08:58:06Z

    On Wed, Mar 8, 2017 at 5:55 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Over in another thread it was pointed out that a patch I submitted
> broke a project rule by including "postgres.h" in a header.  Here is a
> patch to remove it from dsa.h where I made the same mistake, and also
> a case I found in bufmask.h by grepping.
>
> There are also instances in regcustom.h and snowball's header.h -- are
> those special cases?

--- a/src/include/access/bufmask.h
+++ b/src/include/access/bufmask.h
@@ -17,7 +17,6 @@
 #ifndef BUFMASK_H
 #define BUFMASK_H

-#include "postgres.h"
 #include "storage/block.h"
 #include "storage/bufmgr.h"
Oops. This really escaped me...
-- 
Michael

  3. Re: Removing #include "postgres.h" from a couple of headers

    Robert Haas <robertmhaas@gmail.com> — 2017-03-08T13:25:27Z

    On Wed, Mar 8, 2017 at 3:55 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Over in another thread it was pointed out that a patch I submitted
> broke a project rule by including "postgres.h" in a header.  Here is a
> patch to remove it from dsa.h where I made the same mistake, and also
> a case I found in bufmask.h by grepping.

Committed.

> There are also instances in regcustom.h and snowball's header.h -- are
> those special cases?

I will leave this question to someone wiser (or more self-assured) than I.

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

  4. Re: Removing #include "postgres.h" from a couple of headers

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-08T14:33:55Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 8, 2017 at 3:55 AM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> There are also instances in regcustom.h and snowball's header.h -- are
>> those special cases?

> I will leave this question to someone wiser (or more self-assured) than I.

I'm pretty sure I'm to blame for both of those special cases.  The genesis
of both is that we are including these headers from externally-generated
.c files, and it seemed like modifying the .c files would be a bigger
problem than violating the policy.  I am not sure if I hold that position
anymore for the regexp library; our copy has diverged substantially from
Tcl's anyway.  It's still an issue for Snowball, because those .c files
are actually machine-generated by a Snowball-to-C compiler.  We haven't
modified them and probably shouldn't.

If we don't change the code layout, we should probably at least add
comments near these postgres.h inclusions explaining why they're violating
policy.

			regards, tom lane

  5. Re: Removing #include "postgres.h" from a couple of headers

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-08T14:53:50Z

    ... BTW, a bit of grepping shows that there are still significant
violations of this policy with respect to header-driven inclusion of
postgres_fe.h and c.h.  Also, plpgsql is doing it in the unapproved
fashion.  Cleaning these up will take a bit more work, since we'll have
to actually add #includes to some .c files that are currently relying on
those headers to get the core header.  However, if I believe my own
argument, then that's a good thing and we'd better go do it.

Also, ecpglib.h seems like a complete mess: it's relying on symbols
like ENABLE_NLS but there's no certainty as to whether pg_config.h
has been included first.  It won't have been in the case where this
header is read from an ecpg-generated .c file.  So this header will
in fact be interpreted differently in ecpg-generated programs than
in ecpglib and ecpg itself.  Maybe this is okay but it sure smells
like trouble waiting to happen.  I have no desire to try to fix it
myself though.

			regards, tom lane