Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Make the order of the header file includes consistent.

  2. Make the order of the header file includes consistent in backend modules.

  3. Make the order of the header file includes consistent in non-backend modules.

  4. Make the order of the header file includes consistent in contrib modules.

  1. Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-02T09:26:52Z

    Hi,

I noticed that some of the header files inclusion is not ordered as
per the usual standard that is followed.
The attached patch contains the fix for the order in which the header
files are included.
Let me know your thoughts on the same.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  2. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-08T06:35:57Z

    On Wed, Oct 2, 2019 at 2:57 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> I noticed that some of the header files inclusion is not ordered as
> per the usual standard that is followed.
> The attached patch contains the fix for the order in which the header
> files are included.
> Let me know your thoughts on the same.
>

+1.   I think this will make an order of header inclusions consistent
throughout code.   One thing which will be slightly tricky is we might
not be able to back-patch this as some of this belongs to a recent
version(s) and others to older versions as well.   OTOH, I have not
investigated how much of this is relevant to back branches.  I think
most of these will apply to 12, but I am not sure if it is worth the
effort to segregate the changes which apply to back branches.  What do
you think?

 Few minor comments after a quick read:
 #include "lib/ilist.h"
-
+#include "miscadmin.h"

I think we shouldn't remove the extra line as part of the above change.

--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -8,10 +8,8 @@
 #include "postgres_fe.h"

 #include "common.h"
-#include "variables.h"
-
 #include "common/logging.h"
-
+#include "variables.h"

Same as above.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  3. Re: Ordering of header file inclusion

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-08T14:49:36Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Wed, Oct 2, 2019 at 2:57 PM vignesh C <vignesh21@gmail.com> wrote:
>> I noticed that some of the header files inclusion is not ordered as
>> per the usual standard that is followed.
>> The attached patch contains the fix for the order in which the header
>> files are included.
>> Let me know your thoughts on the same.

> +1.

FWIW, I'm not on board with reordering system-header inclusions.
Some platforms have (had?) ordering dependencies for those, and where
that's true, it's seldom alphabetical.  It's only our own headers
where we can safely expect that any arbitrary order will work.

> I think we shouldn't remove the extra line as part of the above change.

I would take out the blank lines between our own #includes.  Those are
totally arbitrary and unnecessary.  The whole point of style rules like
this one is to reduce the differences between the way one person might
write something and the way that someone else might.  Deciding to throw
in a blank line is surely in the realm of unnecessary differences.

			regards, tom lane

  4. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-09T06:07:04Z

    On Tue, Oct 8, 2019 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Wed, Oct 2, 2019 at 2:57 PM vignesh C <vignesh21@gmail.com> wrote:
> >> I noticed that some of the header files inclusion is not ordered as
> >> per the usual standard that is followed.
> >> The attached patch contains the fix for the order in which the header
> >> files are included.
> >> Let me know your thoughts on the same.
>
> > +1.
>
> FWIW, I'm not on board with reordering system-header inclusions.
> Some platforms have (had?) ordering dependencies for those, and where
> that's true, it's seldom alphabetical.  It's only our own headers
> where we can safely expect that any arbitrary order will work.
>

Okay, that makes sense.  However, I noticed that ordering for
system-header inclusions is somewhat random.  For ex. nodeSubPlan.c,
datetime.c, etc. include limits.h first and then math.h whereas
knapsack.c, float.c includes them in reverse order.  There could be
more such inconsistencies and the probable reason is that we don't
have any specific rule, so different people decide to do it
differently.

> > I think we shouldn't remove the extra line as part of the above change.
>
> I would take out the blank lines between our own #includes.
>

Okay, that would be better, but doing it half-heartedly as done in
patch might make it worse.  So, it is better to remove blank lines
between our own #includes in all cases.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  5. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-15T17:27:02Z

    On Wed, Oct 9, 2019 at 11:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 8, 2019 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > On Wed, Oct 2, 2019 at 2:57 PM vignesh C <vignesh21@gmail.com> wrote:
> > >> I noticed that some of the header files inclusion is not ordered as
> > >> per the usual standard that is followed.
> > >> The attached patch contains the fix for the order in which the header
> > >> files are included.
> > >> Let me know your thoughts on the same.
> >
> > > +1.
> >
> > FWIW, I'm not on board with reordering system-header inclusions.
> > Some platforms have (had?) ordering dependencies for those, and where
> > that's true, it's seldom alphabetical.  It's only our own headers
> > where we can safely expect that any arbitrary order will work.
> >
>
> Okay, that makes sense.  However, I noticed that ordering for
> system-header inclusions is somewhat random.  For ex. nodeSubPlan.c,
> datetime.c, etc. include limits.h first and then math.h whereas
> knapsack.c, float.c includes them in reverse order.  There could be
> more such inconsistencies and the probable reason is that we don't
> have any specific rule, so different people decide to do it
> differently.
>
> > > I think we shouldn't remove the extra line as part of the above change.
> >
> > I would take out the blank lines between our own #includes.
> >
>
> Okay, that would be better, but doing it half-heartedly as done in
> patch might make it worse.  So, it is better to remove blank lines
> between our own #includes in all cases.
>
Attached patch contains the fix based on the comments suggested.
I have added/deleted extra lines in certain places so that the
readability is better.
I have removed the duplicate includes of certain header files in same
source file.
In some place postgres header files was getting included as
<postgres_header.h>, I have changed it to "postgres_header.h".
Let me know if any change is required.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  6. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-16T02:40:18Z

    On Tue, Oct 15, 2019 at 10:57 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Oct 9, 2019 at 11:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Oct 8, 2019 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > > On Wed, Oct 2, 2019 at 2:57 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >> I noticed that some of the header files inclusion is not ordered as
> > > >> per the usual standard that is followed.
> > > >> The attached patch contains the fix for the order in which the header
> > > >> files are included.
> > > >> Let me know your thoughts on the same.
> > >
> > > > +1.
> > >
> > > FWIW, I'm not on board with reordering system-header inclusions.
> > > Some platforms have (had?) ordering dependencies for those, and where
> > > that's true, it's seldom alphabetical.  It's only our own headers
> > > where we can safely expect that any arbitrary order will work.
> > >
> >
> > Okay, that makes sense.  However, I noticed that ordering for
> > system-header inclusions is somewhat random.  For ex. nodeSubPlan.c,
> > datetime.c, etc. include limits.h first and then math.h whereas
> > knapsack.c, float.c includes them in reverse order.  There could be
> > more such inconsistencies and the probable reason is that we don't
> > have any specific rule, so different people decide to do it
> > differently.
> >
> > > > I think we shouldn't remove the extra line as part of the above change.
> > >
> > > I would take out the blank lines between our own #includes.
> > >
> >
> > Okay, that would be better, but doing it half-heartedly as done in
> > patch might make it worse.  So, it is better to remove blank lines
> > between our own #includes in all cases.
> >
> Attached patch contains the fix based on the comments suggested.
>

Thanks for working on this.  I will look into this in the coming few
days or during next CF.  Can you please register it for the next CF
(https://commitfest.postgresql.org/25/)?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  7. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-16T03:27:08Z

    On Wed, Oct 16, 2019 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Thanks for working on this.  I will look into this in the coming few
> days or during next CF.  Can you please register it for the next CF
> (https://commitfest.postgresql.org/25/)?
>
Thanks, I have added it to the commitfest.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  8. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-17T11:14:41Z

    On Tue, Oct 15, 2019 at 10:57 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Attached patch contains the fix based on the comments suggested.
> I have added/deleted extra lines in certain places so that the
> readability is better.
>

Hmm, I am not sure if that is better in all cases.  It seems to be
making the code look inconsistent at few places.  See comments below:

1.
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index 4b2186b..45215ba 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -15,6 +15,7 @@
 #include "access/genam.h"
 #include "access/generic_xlog.h"
 #include "access/tableam.h"
+#include "bloom.h"
 #include "catalog/index.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
@@ -23,7 +24,6 @@
 #include "utils/memutils.h"
 #include "utils/rel.h"

-#include "bloom.h"

 PG_MODULE_MAGIC;

diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
index 49e364a..4b9a2b8 100644
--- a/contrib/bloom/blscan.c
+++ b/contrib/bloom/blscan.c
@@ -13,14 +13,14 @@
 #include "postgres.h"

 #include "access/relscan.h"
-#include "pgstat.h"
+#include "bloom.h"
 #include "miscadmin.h"
+#include "pgstat.h"
 #include "storage/bufmgr.h"
 #include "storage/lmgr.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"

-#include "bloom.h"

 /*
  * Begin scan of bloom index.

The above changes lead to one extra line between the last header
include and from where the actual code starts.

2.
diff --git a/contrib/intarray/_int_bool.c b/contrib/intarray/_int_bool.c
index 91e2a80..0d2f667 100644
--- a/contrib/intarray/_int_bool.c
+++ b/contrib/intarray/_int_bool.c
@@ -3,11 +3,11 @@
  */
 #include "postgres.h"

+#include "_int.h"
+
 #include "miscadmin.h"
 #include "utils/builtins.h"

-#include "_int.h"
-
 PG_FUNCTION_INFO_V1(bqarr_in);
 PG_FUNCTION_INFO_V1(bqarr_out);
 PG_FUNCTION_INFO_V1(boolop);
diff --git a/contrib/intarray/_int_gin.c b/contrib/intarray/_int_gin.c
index 7aebfec..d6241d4 100644
--- a/contrib/intarray/_int_gin.c
+++ b/contrib/intarray/_int_gin.c
@@ -3,11 +3,11 @@
  */
 #include "postgres.h"

+#include "_int.h"
+
 #include "access/gin.h"
 #include "access/stratnum.h"

Why extra line after inclusion of _int.h?

3.
diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
index fde8d15..75ad04e 100644
--- a/contrib/intarray/_int_tool.c
+++ b/contrib/intarray/_int_tool.c
@@ -5,10 +5,10 @@

 #include <limits.h>

-#include "catalog/pg_type.h"
-
 #include "_int.h"

+#include "catalog/pg_type.h"
+

Why extra lines after both includes?

4.
diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
index 2a20abe..87ea86c 100644
--- a/contrib/intarray/_intbig_gist.c
+++ b/contrib/intarray/_intbig_gist.c
@@ -3,12 +3,12 @@
  */
 #include "postgres.h"

+#include "_int.h"
+
 #include "access/gist.h"
 #include "access/stratnum.h"
 #include "port/pg_bitutils.h"

-#include "_int.h"
-
 #define GETENTRY(vec,pos) ((GISTTYPE *)
DatumGetPointer((vec)->vector[(pos)].key))
 /*
 ** _intbig methods
diff --git a/contrib/isn/isn.c b/contrib/isn/isn.c
index 0c2cac7..36bb582 100644
--- a/contrib/isn/isn.c
+++ b/contrib/isn/isn.c
@@ -15,9 +15,9 @@
 #include "postgres.h"

 #include "fmgr.h"
+#include "isn.h"
 #include "utils/builtins.h"

-#include "isn.h"

Again extra spaces.  I am not why you have extra spaces in a few cases.

I haven't reviewed it completely, but generally, the changes seem to
be fine.  Please see if you can be consistent in extra space between
includes.  Kindly check the same throughout the patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  9. Re: Ordering of header file inclusion

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-10-19T19:50:03Z

    diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
index f9fe57f..6224735 100644
--- a/contrib/bloom/blcost.c
+++ b/contrib/bloom/blcost.c
@@ -12,10 +12,10 @@
  */
 #include "postgres.h"

+#include "bloom.h"
 #include "fmgr.h"
 #include "utils/selfuncs.h"

-#include "bloom.h"

 /*
  * Estimate cost of bloom index scan.

This class of change I don't like.

The existing arrangement keeps "other" header files separate from the
header file of the module itself.  It seems useful to keep that separate.

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

  10. Re: Ordering of header file inclusion

    Andres Freund <andres@anarazel.de> — 2019-10-19T21:14:19Z

    Hi,

On 2019-10-19 21:50:03 +0200, Peter Eisentraut wrote:
> diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> index f9fe57f..6224735 100644
> --- a/contrib/bloom/blcost.c
> +++ b/contrib/bloom/blcost.c
> @@ -12,10 +12,10 @@
>   */
>  #include "postgres.h"
> 
> +#include "bloom.h"
>  #include "fmgr.h"
>  #include "utils/selfuncs.h"
> 
> -#include "bloom.h"
> 
>  /*
>   * Estimate cost of bloom index scan.
> 
> This class of change I don't like.
> 
> The existing arrangement keeps "other" header files separate from the
> header file of the module itself.  It seems useful to keep that separate.

If we were to do so, we ought to put bloom.h first and clearly seperated
out, not last, as the former makes the bug of the the header not being
standalone more obvious.

I'm -1 on having a policy of putting the headers separate though, I feel
that's too much work, and there's too many cases where it's not that
clear which header that should be.

Greetings,

Andres Freund

  11. Re: Ordering of header file inclusion

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-20T04:53:57Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-10-19 21:50:03 +0200, Peter Eisentraut wrote:
>> This class of change I don't like.
>> The existing arrangement keeps "other" header files separate from the
>> header file of the module itself.  It seems useful to keep that separate.

> If we were to do so, we ought to put bloom.h first and clearly seperated
> out, not last, as the former makes the bug of the the header not being
> standalone more obvious.

We have headerscheck and cpluspluscheck to catch that problem, so I don't
think that it needs to be a reason not to rationalize header inclusion
order.

I don't have a very strong opinion on whether modules outside the core
backend should separate their own headers from core-system headers.
I think there's some argument for that, but it's not something we've
done consistently.  And, as you say, there's no convention as to
where we'd include local headers if we do separate them.

			regards, tom lane

  12. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-20T17:28:03Z

    On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 15, 2019 at 10:57 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Attached patch contains the fix based on the comments suggested.
> > I have added/deleted extra lines in certain places so that the
> > readability is better.
> >
>
> Hmm, I am not sure if that is better in all cases.  It seems to be
> making the code look inconsistent at few places.  See comments below:
>
> 1.
> diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
> index 4b2186b..45215ba 100644
> --- a/contrib/bloom/blinsert.c
> +++ b/contrib/bloom/blinsert.c
> @@ -15,6 +15,7 @@
>  #include "access/genam.h"
>  #include "access/generic_xlog.h"
>  #include "access/tableam.h"
> +#include "bloom.h"
>  #include "catalog/index.h"
>  #include "miscadmin.h"
>  #include "storage/bufmgr.h"
> @@ -23,7 +24,6 @@
>  #include "utils/memutils.h"
>  #include "utils/rel.h"
>
> -#include "bloom.h"
>
>  PG_MODULE_MAGIC;
>
> diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
> index 49e364a..4b9a2b8 100644
> --- a/contrib/bloom/blscan.c
> +++ b/contrib/bloom/blscan.c
> @@ -13,14 +13,14 @@
>  #include "postgres.h"
>
>  #include "access/relscan.h"
> -#include "pgstat.h"
> +#include "bloom.h"
>  #include "miscadmin.h"
> +#include "pgstat.h"
>  #include "storage/bufmgr.h"
>  #include "storage/lmgr.h"
>  #include "utils/memutils.h"
>  #include "utils/rel.h"
>
> -#include "bloom.h"
>
>  /*
>   * Begin scan of bloom index.
>
> The above changes lead to one extra line between the last header
> include and from where the actual code starts.
>
> 2.
> diff --git a/contrib/intarray/_int_bool.c b/contrib/intarray/_int_bool.c
> index 91e2a80..0d2f667 100644
> --- a/contrib/intarray/_int_bool.c
> +++ b/contrib/intarray/_int_bool.c
> @@ -3,11 +3,11 @@
>   */
>  #include "postgres.h"
>
> +#include "_int.h"
> +
>  #include "miscadmin.h"
>  #include "utils/builtins.h"
>
> -#include "_int.h"
> -
>  PG_FUNCTION_INFO_V1(bqarr_in);
>  PG_FUNCTION_INFO_V1(bqarr_out);
>  PG_FUNCTION_INFO_V1(boolop);
> diff --git a/contrib/intarray/_int_gin.c b/contrib/intarray/_int_gin.c
> index 7aebfec..d6241d4 100644
> --- a/contrib/intarray/_int_gin.c
> +++ b/contrib/intarray/_int_gin.c
> @@ -3,11 +3,11 @@
>   */
>  #include "postgres.h"
>
> +#include "_int.h"
> +
>  #include "access/gin.h"
>  #include "access/stratnum.h"
>
> Why extra line after inclusion of _int.h?
>
> 3.
> diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
> index fde8d15..75ad04e 100644
> --- a/contrib/intarray/_int_tool.c
> +++ b/contrib/intarray/_int_tool.c
> @@ -5,10 +5,10 @@
>
>  #include <limits.h>
>
> -#include "catalog/pg_type.h"
> -
>  #include "_int.h"
>
> +#include "catalog/pg_type.h"
> +
>
> Why extra lines after both includes?
>
> 4.
> diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
> index 2a20abe..87ea86c 100644
> --- a/contrib/intarray/_intbig_gist.c
> +++ b/contrib/intarray/_intbig_gist.c
> @@ -3,12 +3,12 @@
>   */
>  #include "postgres.h"
>
> +#include "_int.h"
> +
>  #include "access/gist.h"
>  #include "access/stratnum.h"
>  #include "port/pg_bitutils.h"
>
> -#include "_int.h"
> -
>  #define GETENTRY(vec,pos) ((GISTTYPE *)
> DatumGetPointer((vec)->vector[(pos)].key))
>  /*
>  ** _intbig methods
> diff --git a/contrib/isn/isn.c b/contrib/isn/isn.c
> index 0c2cac7..36bb582 100644
> --- a/contrib/isn/isn.c
> +++ b/contrib/isn/isn.c
> @@ -15,9 +15,9 @@
>  #include "postgres.h"
>
>  #include "fmgr.h"
> +#include "isn.h"
>  #include "utils/builtins.h"
>
> -#include "isn.h"
>
> Again extra spaces.  I am not why you have extra spaces in a few cases.
>
> I haven't reviewed it completely, but generally, the changes seem to
> be fine.  Please see if you can be consistent in extra space between
> includes.  Kindly check the same throughout the patch.
>
Thanks for reviewing the patch.
I have made an updated patch with comments you have suggested.
I have split the patch into 3 patches so that the review can be simpler.
This patch also includes the changes suggested by Peter & Andres.
I had just seen seen Tom Lane's suggestions regarding submodule header
file, this patch contains fix based on Andres suggestions. Let me know
if that need to be changed, I can update it.
Should we make this changes only in master branch or should we make in
back branches also. If we decide for back branches, I will check if
this patch can apply in back branches and if this patch cannot  be
directly applied I can make separate patch for the back branch and
send.
Please let me know your suggestions for any changes.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  13. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-20T17:31:00Z

    On Sun, Oct 20, 2019 at 1:20 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> index f9fe57f..6224735 100644
> --- a/contrib/bloom/blcost.c
> +++ b/contrib/bloom/blcost.c
> @@ -12,10 +12,10 @@
>   */
>  #include "postgres.h"
>
> +#include "bloom.h"
>  #include "fmgr.h"
>  #include "utils/selfuncs.h"
>
> -#include "bloom.h"
>
>  /*
>   * Estimate cost of bloom index scan.
>
> This class of change I don't like.
>
> The existing arrangement keeps "other" header files separate from the
> header file of the module itself.  It seems useful to keep that separate.
>
Thanks Peter for your thoughts, I have modified the changes based on
your suggestions.
I have included the module header file in the beginning.
The changes are attached in the previous mail.

Please let me know your suggestions for any changes.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  14. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-20T17:32:31Z

    On Sun, Oct 20, 2019 at 2:44 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-10-19 21:50:03 +0200, Peter Eisentraut wrote:
> > diff --git a/contrib/bloom/blcost.c b/contrib/bloom/blcost.c
> > index f9fe57f..6224735 100644
> > --- a/contrib/bloom/blcost.c
> > +++ b/contrib/bloom/blcost.c
> > @@ -12,10 +12,10 @@
> >   */
> >  #include "postgres.h"
> >
> > +#include "bloom.h"
> >  #include "fmgr.h"
> >  #include "utils/selfuncs.h"
> >
> > -#include "bloom.h"
> >
> >  /*
> >   * Estimate cost of bloom index scan.
> >
> > This class of change I don't like.
> >
> > The existing arrangement keeps "other" header files separate from the
> > header file of the module itself.  It seems useful to keep that separate.
>
> If we were to do so, we ought to put bloom.h first and clearly seperated
> out, not last, as the former makes the bug of the the header not being
> standalone more obvious.
>
> I'm -1 on having a policy of putting the headers separate though, I feel
> that's too much work, and there's too many cases where it's not that
> clear which header that should be.
>
Thanks Andres for reviewing the changes, I have modified the changes
based on your suggestions.
I have included the module header file in the beginning.
The changes are attached in the previous mail.

Please let me know your suggestions for any changes.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  15. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-21T03:17:04Z

    On Sun, Oct 20, 2019 at 10:58 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > I haven't reviewed it completely, but generally, the changes seem to
> > be fine.  Please see if you can be consistent in extra space between
> > includes.  Kindly check the same throughout the patch.
> >
> Thanks for reviewing the patch.
> I have made an updated patch with comments you have suggested.
> I have split the patch into 3 patches so that the review can be simpler.
> This patch also includes the changes suggested by Peter & Andres.
> I had just seen seen Tom Lane's suggestions regarding submodule header
> file, this patch contains fix based on Andres suggestions. Let me know
> if that need to be changed, I can update it.
>

AFAICS, none of Andres or Tom seems to be in favor of separating
module headers.  I am also not sure if we should try to make sure of
that in every case.

> Should we make this changes only in master branch or should we make in
> back branches also.
>

I am in favor of doing this only for HEAD, but I am fine if others
want to see for back branches as well and you can prepare the patches
for the same.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  16. Re: Ordering of header file inclusion

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-21T03:22:21Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Sun, Oct 20, 2019 at 10:58 PM vignesh C <vignesh21@gmail.com> wrote:
>> Should we make this changes only in master branch or should we make in
>> back branches also.

> I am in favor of doing this only for HEAD, but I am fine if others
> want to see for back branches as well and you can prepare the patches
> for the same.

There is no good reason to back-patch this.

			regards, tom lane

  17. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-21T17:34:40Z

    On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Oct 20, 2019 at 10:58 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, Oct 17, 2019 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > I haven't reviewed it completely, but generally, the changes seem to
> > > be fine.  Please see if you can be consistent in extra space between
> > > includes.  Kindly check the same throughout the patch.
> > >
> > Thanks for reviewing the patch.
> > I have made an updated patch with comments you have suggested.
> > I have split the patch into 3 patches so that the review can be simpler.
> > This patch also includes the changes suggested by Peter & Andres.
> > I had just seen seen Tom Lane's suggestions regarding submodule header
> > file, this patch contains fix based on Andres suggestions. Let me know
> > if that need to be changed, I can update it.
> >
>
> AFAICS, none of Andres or Tom seems to be in favor of separating
> module headers.  I am also not sure if we should try to make sure of
> that in every case.
>
Thanks for the suggestions.
Updated patch contains the fix based on Tom Lane's Suggestion.
Let me know your thoughts for further revision if required.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  18. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-22T07:26:46Z

    On Mon, Oct 21, 2019 at 11:04 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> Thanks for the suggestions.
> Updated patch contains the fix based on Tom Lane's Suggestion.
> Let me know your thoughts for further revision if required.
>

Few comments on 0001-Ordering-of-header-files-in-contrib-dir-oct21.patch
-----------------------------------------------------------------------------------------------------------
1.
--- a/contrib/isn/isn.c
+++ b/contrib/isn/isn.c
@@ -15,9 +15,9 @@
 #include "postgres.h"

 #include "fmgr.h"
+#include "isn.h"
 #include "utils/builtins.h"

-#include "isn.h"
 #include "EAN13.h"
 #include "ISBN.h"
 #include "ISMN.h"

Why only "isn.h" is moved and not others?

2.
+++ b/contrib/pgcrypto/px-crypt.c
@@ -31,9 +31,8 @@

 #include "postgres.h"

-#include "px.h"
 #include "px-crypt.h"
-
+#include "px.h"

I think such ordering was fine.  Forex. see, hash.c (hash.h was
included first and then hash_xlog.h).

3.
+#include "_int.h"
 #include "access/gist.h"
 #include "access/stratnum.h"

-#include "_int.h"
-

Do we need to give preference to '_'?  Is it being done somewhere
else?  It is not that this is wrong, just that I am not sure about
this.

4.
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -8,6 +8,7 @@
 #include "access/htup_details.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
+#include "hstore.h"
 #include "lib/stringinfo.h"
 #include "libpq/pqformat.h"
 #include "utils/builtins.h"
@@ -18,7 +19,6 @@
 #include "utils/memutils.h"
 #include "utils/typcache.h"

-#include "hstore.h"

 PG_MODULE_MAGIC;

This created an extra white line.

5.
While reviewing, I noticed that in contrib/intarray/_int_op.c, there
is an extra white line between postgres.h and its first include.  I
think we can make that as well consistent.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  19. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-22T10:11:51Z

    On Tue, Oct 22, 2019 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Oct 21, 2019 at 11:04 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > Thanks for the suggestions.
> > Updated patch contains the fix based on Tom Lane's Suggestion.
> > Let me know your thoughts for further revision if required.
> >

This patch series has broadly changed the code to organize the header
includes in alphabetic order.  It also makes sure that all files first
includes 'postgres.h'/'postgres_fe.h', system header includes and then
Postgres header includes.

It also has a change where it seems that for local header includes, we
have used '<>' whereas quotes ("") should have been used.  See,
ecpg/compatlib/informix.c.

I am planning to commit this as multiple commits (a. contrib modules,
b. non-backend changes and c. backend changes) as there is some risk
of buildfarm break.  From my side, I will ensure that everything is
passing on windows and centos.  Any objections to this plan?

Review for 0003-Ordering-of-header-files-remaining-dir-oct21
-----------------------------------------------------------------------------------------
1.
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -19,18 +19,16 @@
 #include <sys/select.h>
 #endif

-/* local
includes */
-#include "streamutil.h"
-
 #include "access/xlog_internal.h"
-#include "common/file_perm.h"
 #include "common/fe_memutils.h"
+#include
"common/file_perm.h"
 #include "common/logging.h"
 #include "getopt_long.h"
 #include "libpq-fe.h"
 #include "libpq/pqsignal.h"
 #include "pqexpbuffer.h"

+#include "streamutil.h"

Extra space before streamutil.h include.

2.
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -21,11 +21,6 @@
 #include <time.h>
 #include <unistd.h>

-#include
"libpq-fe.h"
-#include "libpq-int.h"
-#include "fe-auth.h"
-#include "pg_config_paths.h"
-
 #ifdef WIN32
 #include "win32.h"
 #ifdef _WIN32_IE
@@ -74,10
+69,13 @@ static int ldapServiceLookup(const char *purl,
PQconninfoOption *options,
 #include "common/link-canary.h"
 #include "common/scram-
common.h"
 #include "common/string.h"
+#include "fe-auth.h"
+#include "libpq-fe.h"
+#include "libpq-int.h"
 #include "mb/pg_wchar.h"
+#include
"pg_config_paths.h"

After this change, the Windows build is failing for me.  You forgot to
move the below code:
#ifdef USE_LDAP
#ifdef WIN32
#include <winldap.h>
#else
/* OpenLDAP deprecates RFC 1823, but we want standard conformance */
#define LDAP_DEPRECATED 1
#include <ldap.h>
typedef struct timeval LDAP_TIMEVAL;
#endif
static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
  PQExpBuffer errorMessage);
#endif

All this needs to be moved after all the includes.

3.
 /* ScanKeywordList lookup data for ECPG keywords */
 #include "ecpg_kwlist_d.h"
+#include "preproc_extern.h"
+#include "preproc.h"

I think preproc.h include should be before preproc_extern.h due to the
reason mentioned earlier.

4.
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -22,24 +22,22 @@
  */
 #include "postgres.h"

-/* These are always necessary for a bgworker */
+/* these headers are used by this particular worker's code */
+#include "access/xact.h"
+#include "executor/spi.h"
+#include "fmgr.h"
+#include "lib/stringinfo.h"
 #include "miscadmin.h"
+#include "pgstat.h"
 #include "postmaster/bgworker.h"
 #include "storage/ipc.h"
 #include "storage/latch.h"
 #include "storage/lwlock.h"
 #include "storage/proc.h"
 #include "storage/shmem.h"
-
-/* these headers are used by this particular worker's code */
-#include "access/xact.h"
-#include "executor/spi.h"

I am skeptical of this change as it is very clearly written in
comments the reason why header includes are separated.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  20. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-22T17:22:15Z

    On Tue, Oct 22, 2019 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Few comments on 0001-Ordering-of-header-files-in-contrib-dir-oct21.patch
> -----------------------------------------------------------------------------------------------------------
> 1.
> --- a/contrib/isn/isn.c
> +++ b/contrib/isn/isn.c
> @@ -15,9 +15,9 @@
>  #include "postgres.h"
>
>  #include "fmgr.h"
> +#include "isn.h"
>  #include "utils/builtins.h"
>
> -#include "isn.h"
>  #include "EAN13.h"
>  #include "ISBN.h"
>  #include "ISMN.h"
>
> Why only "isn.h" is moved and not others?
>
Fixed.
Order is based on ascii table. Upper case letters will come before
lower case letters.
> 2.
> +++ b/contrib/pgcrypto/px-crypt.c
> @@ -31,9 +31,8 @@
>
>  #include "postgres.h"
>
> -#include "px.h"
>  #include "px-crypt.h"
> -
> +#include "px.h"
>
> I think such ordering was fine.  Forex. see, hash.c (hash.h was
> included first and then hash_xlog.h).
>
Order is based on ascii table.
Ascii value for "." is 46, Ascii value for "-" is 45.
Hence have placed like:
#include "px-crypt.h"
#include "px.h"
Not make any changes for this. If still required I can modify.
> 3.
> +#include "_int.h"
>  #include "access/gist.h"
>  #include "access/stratnum.h"
>
> -#include "_int.h"
> -
>
> Do we need to give preference to '_'?  Is it being done somewhere
> else?  It is not that this is wrong, just that I am not sure about
> this.
>
The changes are done based on ascii table.
Ascii value of "_" is 95.
Ascii value of "a" is 97.
Hence _int.h is place before access/gist.h.
I have not made any changes for this. If still required I can modify.
> 4.
> --- a/contrib/hstore/hstore_io.c
> +++ b/contrib/hstore/hstore_io.c
> @@ -8,6 +8,7 @@
>  #include "access/htup_details.h"
>  #include "catalog/pg_type.h"
>  #include "funcapi.h"
> +#include "hstore.h"
>  #include "lib/stringinfo.h"
>  #include "libpq/pqformat.h"
>  #include "utils/builtins.h"
> @@ -18,7 +19,6 @@
>  #include "utils/memutils.h"
>  #include "utils/typcache.h"
>
> -#include "hstore.h"
>
>  PG_MODULE_MAGIC;
>
> This created an extra white line.
>
Fixed.
> 5.
> While reviewing, I noticed that in contrib/intarray/_int_op.c, there
> is an extra white line between postgres.h and its first include.  I
> think we can make that as well consistent.
>
Fixed.
Thanks for the comments.
Attached patch has the updated changes.


Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  21. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-22T17:35:12Z

    On Tue, Oct 22, 2019 at 3:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Review for 0003-Ordering-of-header-files-remaining-dir-oct21
> -----------------------------------------------------------------------------------------
> 1.
> --- a/src/bin/pg_basebackup/pg_recvlogical.c
> +++ b/src/bin/pg_basebackup/pg_recvlogical.c
> @@ -19,18 +19,16 @@
>  #include <sys/select.h>
>  #endif
>
> -/* local
> includes */
> -#include "streamutil.h"
> -
>  #include "access/xlog_internal.h"
> -#include "common/file_perm.h"
>  #include "common/fe_memutils.h"
> +#include
> "common/file_perm.h"
>  #include "common/logging.h"
>  #include "getopt_long.h"
>  #include "libpq-fe.h"
>  #include "libpq/pqsignal.h"
>  #include "pqexpbuffer.h"
>
> +#include "streamutil.h"
>
> Extra space before streamutil.h include.
>
Fixed.
> 2.
> --- a/src/interfaces/libpq/fe-connect.c
> +++ b/src/interfaces/libpq/fe-connect.c
> @@ -21,11 +21,6 @@
>  #include <time.h>
>  #include <unistd.h>
>
> -#include
> "libpq-fe.h"
> -#include "libpq-int.h"
> -#include "fe-auth.h"
> -#include "pg_config_paths.h"
> -
>  #ifdef WIN32
>  #include "win32.h"
>  #ifdef _WIN32_IE
> @@ -74,10
> +69,13 @@ static int ldapServiceLookup(const char *purl,
> PQconninfoOption *options,
>  #include "common/link-canary.h"
>  #include "common/scram-
> common.h"
>  #include "common/string.h"
> +#include "fe-auth.h"
> +#include "libpq-fe.h"
> +#include "libpq-int.h"
>  #include "mb/pg_wchar.h"
> +#include
> "pg_config_paths.h"
>
> After this change, the Windows build is failing for me.  You forgot to
> move the below code:
> #ifdef USE_LDAP
> #ifdef WIN32
> #include <winldap.h>
> #else
> /* OpenLDAP deprecates RFC 1823, but we want standard conformance */
> #define LDAP_DEPRECATED 1
> #include <ldap.h>
> typedef struct timeval LDAP_TIMEVAL;
> #endif
> static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
>   PQExpBuffer errorMessage);
> #endif
>
> All this needs to be moved after all the includes.
>
Fixed. I don't have windows environment, let me know if you still face
some issue.
> 3.
>  /* ScanKeywordList lookup data for ECPG keywords */
>  #include "ecpg_kwlist_d.h"
> +#include "preproc_extern.h"
> +#include "preproc.h"
>
> I think preproc.h include should be before preproc_extern.h due to the
> reason mentioned earlier.
>
For this file the earlier order was also like that.
As per the ordering preproc_extern.h should be before prepr.h.
But the preproc.h has some dependency on preproc_extern.h.
Not made any changes for this.
Same is the case in c_keywords.c also.
> 4.
> --- a/src/test/modules/worker_spi/worker_spi.c
> +++ b/src/test/modules/worker_spi/worker_spi.c
> @@ -22,24 +22,22 @@
>   */
>  #include "postgres.h"
>
> -/* These are always necessary for a bgworker */
> +/* these headers are used by this particular worker's code */
> +#include "access/xact.h"
> +#include "executor/spi.h"
> +#include "fmgr.h"
> +#include "lib/stringinfo.h"
>  #include "miscadmin.h"
> +#include "pgstat.h"
>  #include "postmaster/bgworker.h"
>  #include "storage/ipc.h"
>  #include "storage/latch.h"
>  #include "storage/lwlock.h"
>  #include "storage/proc.h"
>  #include "storage/shmem.h"
> -
> -/* these headers are used by this particular worker's code */
> -#include "access/xact.h"
> -#include "executor/spi.h"
>
> I am skeptical of this change as it is very clearly written in
> comments the reason why header includes are separated.
Fixed. Have reverted this change.
Attached patch has the updated changes.
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com

  22. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-10-23T04:42:32Z

    On Tue, Oct 22, 2019 at 11:05 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Oct 22, 2019 at 3:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Review for 0003-Ordering-of-header-files-remaining-dir-oct21
> > -----------------------------------------------------------------------------------------
> > 1.
> > --- a/src/bin/pg_basebackup/pg_recvlogical.c
> > +++ b/src/bin/pg_basebackup/pg_recvlogical.c
> > @@ -19,18 +19,16 @@
I have compiled and verified make check & make check-world  in the following:
CentOS Linux release 7.7.1908
Red Hat Enterprise Linux Server release 7.1

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  23. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-23T04:53:05Z

    On Tue, Oct 22, 2019 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 22, 2019 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Oct 21, 2019 at 11:04 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Mon, Oct 21, 2019 at 8:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> This patch series has broadly changed the code to organize the header
> includes in alphabetic order.  It also makes sure that all files first
> includes 'postgres.h'/'postgres_fe.h', system header includes and then
> Postgres header includes.
>
> It also has a change where it seems that for local header includes, we
> have used '<>' whereas quotes ("") should have been used.  See,
> ecpg/compatlib/informix.c.
>
> I am planning to commit this as multiple commits (a. contrib modules,
> b. non-backend changes and c. backend changes) as there is some risk
> of buildfarm break.  From my side, I will ensure that everything is
> passing on windows and centos.  Any objections to this plan?
>

Attached are patches for (a) and (b) after another round of review and
fixes by Vignesh.   I am planning to commit the first one (a) tomorrow
morning and then if everything is fine on buildfarm, I will commit the
second one (b) and once both are good, I will look into the third one
(c).   Another pair of eyes on these patches would be good.

Just to be clear,  the basic rule we follow here is to always first
include 'postgres.h' or 'postgres_fe.h' whichever is applicable, then
system header includes and then Postgres header includes.  In this, we
also follow that all the Postgres header includes are in order based
on their ASCII value.  We generally follow these rules, but the code
has deviated in many places.  These commits make these rules
consistent for the entire code.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  24. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-24T09:13:48Z

    On Wed, Oct 23, 2019 at 10:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Attached are patches for (a) and (b) after another round of review and
> fixes by Vignesh.   I am planning to commit the first one (a) tomorrow
> morning and then if everything is fine on buildfarm, I will commit the
> second one (b) and once both are good, I will look into the third one
> (c).   Another pair of eyes on these patches would be good.
>

I have pushed the first one.  I'll wait for some time and probably
commit the second one tomorrow.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  25. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-25T09:58:47Z

    On Thu, Oct 24, 2019 at 2:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Oct 23, 2019 at 10:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Attached are patches for (a) and (b) after another round of review and
> > fixes by Vignesh.   I am planning to commit the first one (a) tomorrow
> > morning and then if everything is fine on buildfarm, I will commit the
> > second one (b) and once both are good, I will look into the third one
> > (c).   Another pair of eyes on these patches would be good.
> >
>
> I have pushed the first one.  I'll wait for some time and probably
> commit the second one tomorrow.
>

I have committed the second one as well.  Now, the remaining patch is
for the entire backend.  I will pick it up after a few days.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  26. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-11-02T02:12:06Z

    On Fri, Oct 25, 2019 at 3:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 24, 2019 at 2:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Oct 23, 2019 at 10:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > Attached are patches for (a) and (b) after another round of review and
> > > fixes by Vignesh.   I am planning to commit the first one (a) tomorrow
> > > morning and then if everything is fine on buildfarm, I will commit the
> > > second one (b) and once both are good, I will look into the third one
> > > (c).   Another pair of eyes on these patches would be good.
> > >
> >
> > I have pushed the first one.  I'll wait for some time and probably
> > commit the second one tomorrow.
> >
>
> I have committed the second one as well.  Now, the remaining patch is
> for the entire backend.  I will pick it up after a few days.
>
Thanks Amit for committing the changes.
I found couple of more inconsistencies,  the attached patch includes
the fix for the same.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  27. Re: Ordering of header file inclusion

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2019-11-08T08:52:32Z

    On Sat, Nov 2, 2019 at 7:42 AM vignesh C <vignesh21@gmail.com> wrote:
>
> >
> Thanks Amit for committing the changes.
> I found couple of more inconsistencies,  the attached patch includes
> the fix for the same.
>
Thanks for the patch. It seems you've to rebase the patch as it
doesn't apply on the latest HEAD. Apart from that, the changes looks
good to me.



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  28. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-11-10T12:00:44Z

    On Fri, Nov 8, 2019 at 2:22 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
>
> On Sat, Nov 2, 2019 at 7:42 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > >
> > Thanks Amit for committing the changes.
> > I found couple of more inconsistencies,  the attached patch includes
> > the fix for the same.
> >
> Thanks for the patch. It seems you've to rebase the patch as it
> doesn't apply on the latest HEAD. Apart from that, the changes looks
> good to me.
>

Thanks Kuntal for reviewing the patch. I have attached the patch which
has been rebased on the latest HEAD.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  29. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-11T02:23:53Z

    On Sun, Nov 10, 2019 at 5:30 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, Nov 8, 2019 at 2:22 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
> >
> > On Sat, Nov 2, 2019 at 7:42 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > >
> > > Thanks Amit for committing the changes.
> > > I found couple of more inconsistencies,  the attached patch includes
> > > the fix for the same.
> > >
> > Thanks for the patch. It seems you've to rebase the patch as it
> > doesn't apply on the latest HEAD. Apart from that, the changes looks
> > good to me.
> >
>
> Thanks Kuntal for reviewing the patch. I have attached the patch which
> has been rebased on the latest HEAD.
>

Thanks for the latest patch.  I will look into this today.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  30. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-11T06:05:53Z

    On Sun, Nov 10, 2019 at 5:30 PM vignesh C <vignesh21@gmail.com> wrote:
>
[review_latest_patch]:

Do we want to consider the ordering of map file inclusions as well
(see the changes pointed out below)?  If so, what all we should
validate, is compilation of these modules sufficient?  Tom, anyone, do
you have any opinion on this?

1.
 utf8_and_cyrillic.c

 #include "fmgr.h"
 #include "mb/pg_wchar.h"
-#include "../../Unicode/utf8_to_koi8r.map"
 #include "../../Unicode/koi8r_to_utf8.map"
-#include "../../Unicode/utf8_to_koi8u.map"
 #include "../../Unicode/koi8u_to_utf8.map"
+#include "../../Unicode/utf8_to_koi8r.map"
+#include "../../Unicode/utf8_to_koi8u.map"

 PG_MODULE_MAGIC;

2.
utf8_and_iso8859.c
..
 #include "../../Unicode/iso8859_13_to_utf8.map"
 #include "../../Unicode/iso8859_14_to_utf8.map"
 #include "../../Unicode/iso8859_15_to_utf8.map"
+#include "../../Unicode/iso8859_16_to_utf8.map"
 #include "../../Unicode/iso8859_2_to_utf8.map"
 #include "../../Unicode/iso8859_3_to_utf8.map"
 #include "../../Unicode/iso8859_4_to_utf8.map"
@@ -39,7 +41,6 @@
 #include "../../Unicode/utf8_to_iso8859_7.map"
 #include "../../Unicode/utf8_to_iso8859_8.map"
 #include "../../Unicode/utf8_to_iso8859_9.map"
-#include "../../Unicode/iso8859_16_to_utf8.map"


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  31. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-11-12T01:03:40Z

    On Mon, Nov 11, 2019 at 11:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Nov 10, 2019 at 5:30 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> [review_latest_patch]:
>
> Do we want to consider the ordering of map file inclusions as well
> (see the changes pointed out below)?  If so, what all we should
> validate, is compilation of these modules sufficient?  Tom, anyone, do
> you have any opinion on this?
>

Even I don't know how to validate the above changes by some test
application, other than by compiling.

> 1.
>  utf8_and_cyrillic.c
>
>  #include "fmgr.h"
>  #include "mb/pg_wchar.h"
> -#include "../../Unicode/utf8_to_koi8r.map"
>  #include "../../Unicode/koi8r_to_utf8.map"
> -#include "../../Unicode/utf8_to_koi8u.map"
>  #include "../../Unicode/koi8u_to_utf8.map"
> +#include "../../Unicode/utf8_to_koi8r.map"
> +#include "../../Unicode/utf8_to_koi8u.map"
>
>  PG_MODULE_MAGIC;
>
> 2.
> utf8_and_iso8859.c
> ..
>  #include "../../Unicode/iso8859_13_to_utf8.map"
>  #include "../../Unicode/iso8859_14_to_utf8.map"
>  #include "../../Unicode/iso8859_15_to_utf8.map"
> +#include "../../Unicode/iso8859_16_to_utf8.map"
>  #include "../../Unicode/iso8859_2_to_utf8.map"
>  #include "../../Unicode/iso8859_3_to_utf8.map"
>  #include "../../Unicode/iso8859_4_to_utf8.map"
> @@ -39,7 +41,6 @@
>  #include "../../Unicode/utf8_to_iso8859_7.map"
>  #include "../../Unicode/utf8_to_iso8859_8.map"
>  #include "../../Unicode/utf8_to_iso8859_9.map"
> -#include "../../Unicode/iso8859_16_to_utf8.map"
>

Thanks Amit for your comments. Please find the updated patch which
does not include the changes mentioned above. I will post a separate
patch for these changes based on the response from others.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  32. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-12T05:49:11Z

    On Tue, Nov 12, 2019 at 6:33 AM vignesh C <vignesh21@gmail.com> wrote:
>
>
> Thanks Amit for your comments. Please find the updated patch which
> does not include the changes mentioned above.
>

Thanks for working on this.  I have pushed your latest patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  33. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-11-16T01:31:43Z

    On Tue, Nov 12, 2019 at 11:19 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Nov 12, 2019 at 6:33 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> >
> > Thanks Amit for your comments. Please find the updated patch which
> > does not include the changes mentioned above.
> >
>
> Thanks for working on this.  I have pushed your latest patch.
>

Thanks Amit for pushing the patch. I have re-verified and found that
changes need  to be done in few more places. The main changes are made
in the header file and plpython source files. The attached patch
handles the same. I have verified make check and make check-world
including --with-python & --with-perl in the following:
CentOS Linux release 7.7.1908
Red Hat Enterprise Linux Server release 7.1

I have verified including --llvm in CentOS Linux release 7.7.1908.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  34. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-21T08:40:59Z

    On Sat, Nov 16, 2019 at 7:01 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Nov 12, 2019 at 11:19 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Nov 12, 2019 at 6:33 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > >
> > > Thanks Amit for your comments. Please find the updated patch which
> > > does not include the changes mentioned above.
> > >
> >
> > Thanks for working on this.  I have pushed your latest patch.
> >
>
> Thanks Amit for pushing the patch. I have re-verified and found that
> changes need  to be done in few more places. The main changes are made
> in the header file and plpython source files. The attached patch
> handles the same. I have verified make check and make check-world
> including --with-python & --with-perl in the following:
> CentOS Linux release 7.7.1908
> Red Hat Enterprise Linux Server release 7.1
>
> I have verified including --llvm in CentOS Linux release 7.7.1908.
>

Thanks for finding the remaining places, the patch looks good to me.
I hope this covers the entire code.  BTW, are you using some script to
find this or is this a result of manual inspection of code?  I have
modified the commit message in the attached patch.  I will commit this
early next week unless someone else wants to review it.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  35. Re: Ordering of header file inclusion

    vignesh C <vignesh21@gmail.com> — 2019-11-22T04:14:20Z

    On Thu, Nov 21, 2019 at 2:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Nov 16, 2019 at 7:01 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, Nov 12, 2019 at 11:19 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Nov 12, 2019 at 6:33 AM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > >
> > > > Thanks Amit for your comments. Please find the updated patch which
> > > > does not include the changes mentioned above.
> > > >
> > >
> > > Thanks for working on this.  I have pushed your latest patch.
> > >
> >
> > Thanks Amit for pushing the patch. I have re-verified and found that
> > changes need  to be done in few more places. The main changes are made
> > in the header file and plpython source files. The attached patch
> > handles the same. I have verified make check and make check-world
> > including --with-python & --with-perl in the following:
> > CentOS Linux release 7.7.1908
> > Red Hat Enterprise Linux Server release 7.1
> >
> > I have verified including --llvm in CentOS Linux release 7.7.1908.
> >
>
> Thanks for finding the remaining places, the patch looks good to me.
> I hope this covers the entire code.  BTW, are you using some script to
> find this or is this a result of manual inspection of code?  I have
> modified the commit message in the attached patch.  I will commit this
> early next week unless someone else wants to review it.
>

I have used script to verify if the inclusions are sorted. There are
few files which I did not modify intentionally, they are mainly like
the below type as in uuid-ossp.c:
#include "postgres.h"

#include "fmgr.h"
#include "port/pg_bswap.h"
#include "utils/builtins.h"
#include "utils/uuid.h"

/*
 * It's possible that there's more than one uuid.h header file present.
 * We expect configure to set the HAVE_ symbol for only the one we want.
 *
 * BSD includes a uuid_hash() function that conflicts with the one in
 * builtins.h; we #define it out of the way.
 */
#define uuid_hash bsd_uuid_hash

#if defined(HAVE_UUID_H)
#include <uuid.h>
#elif defined(HAVE_OSSP_UUID_H)
#include <ossp/uuid.h>
#elif defined(HAVE_UUID_UUID_H)
#include <uuid/uuid.h>


After the inclusion they have define and further include based on #if
defined. In few cases I had seen the include happens at the end of the
file like in regcomp.c as there may be impact. I felt it is better not
to change these files. Let me know your thoughts on the same.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  36. Re: Ordering of header file inclusion

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-22T14:37:07Z

    vignesh C <vignesh21@gmail.com> writes:
> After the inclusion they have define and further include based on #if
> defined. In few cases I had seen the include happens at the end of the
> file like in regcomp.c as there may be impact. I felt it is better not
> to change these files. Let me know your thoughts on the same.

I think the point of this patch series is just to make cosmetic
adjustments in places where people have randomly failed to maintain
alphabetic order of a consecutive group of #include's.  Messing with
examples like the above is way out of scope, if you ask me --- it
entails more analysis, and more risk of breakage, than a purely
cosmetic goal is worth.

			regards, tom lane

  37. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-23T05:53:26Z

    On Fri, Nov 22, 2019 at 8:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> vignesh C <vignesh21@gmail.com> writes:
> > After the inclusion they have define and further include based on #if
> > defined. In few cases I had seen the include happens at the end of the
> > file like in regcomp.c as there may be impact. I felt it is better not
> > to change these files. Let me know your thoughts on the same.
>
> I think the point of this patch series is just to make cosmetic
> adjustments in places where people have randomly failed to maintain
> alphabetic order of a consecutive group of #include's.  Messing with
> examples like the above is way out of scope, if you ask me --- it
> entails more analysis, and more risk of breakage, than a purely
> cosmetic goal is worth.
>

+1.  I agree with what Tom said, so let's leave such things as it is.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  38. Re: Ordering of header file inclusion

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-25T12:08:09Z

    On Thu, Nov 21, 2019 at 2:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Thanks for finding the remaining places, the patch looks good to me.
> I hope this covers the entire code.  BTW, are you using some script to
> find this or is this a result of manual inspection of code?  I have
> modified the commit message in the attached patch.  I will commit this
> early next week unless someone else wants to review it.
>

Pushed.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com