Thread: Ordering of header file inclusion
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
Attachment
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
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
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
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
Attachment
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
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
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
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
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
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
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
Attachment
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
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
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
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
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
Attachment
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
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
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
Attachment
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
Attachment
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
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
Attachment
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
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
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
Attachment
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
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
Attachment
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
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
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
Attachment
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
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
Attachment
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
Attachment
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
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
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
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