Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> So I really would like to get a pgindent run done.  Any objections to
> doing it sometime RSN?  It is of course possible that it might make
> something that we want to revert later harder to revert, but I think
> we should just accept that risk and move forward.

Now that we bit the bullet on 137805f89, I do not think there's anything
else with a really high probability of being reverted.  Might as well do
the run.  Please note that typedefs.list is already out of date.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 11:31 AM, Robert Haas  wrote:
> On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> OK, I committed this.  Barring objections, I'll go ahead and pgindent
>>> the whole tree tomorrow.  If we're going to revert anything big then
>>> we might want to hold off, but otherwise I think its better to get
>>> this done sooner rather than later.
>>
>> Well, there are at least two patchsets we're actively discussing
>> reverting, so I think this should wait till those decisions are resolved.
>
> OK, but that may well mean we don't get this done before beta1, which
> I think is a bummer, but oh well.

So I really would like to get a pgindent run done.  Any objections to
doing it sometime RSN?  It is of course possible that it might make
something that we want to revert later harder to revert, but I think
we should just accept that risk and move forward.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
>> Well, there are at least two patchsets we're actively discussing
>> reverting, so I think this should wait till those decisions are resolved.

> OK, but that may well mean we don't get this done before beta1, which
> I think is a bummer, but oh well.

pgindent is reliable enough that I'm not really worried about running
it post-beta.  Obviously sooner is better than later, to give authors
of 9.7 patches more time to rebase; but I do not think we should give
ourselves extra work just to do it a little sooner.

> There are a lot more than 2 patchsets that are busted at the moment,
> unfortunately, but I assume you are referring to "snapshot too old"
> and "Use Foreign Key relationships to infer multi-column join
> selectivity".

Yeah, those are the ones I'm thinking of.  I've not heard serious
proposals to revert any others, have you?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> OK, I committed this.  Barring objections, I'll go ahead and pgindent
>> the whole tree tomorrow.  If we're going to revert anything big then
>> we might want to hold off, but otherwise I think its better to get
>> this done sooner rather than later.
>
> Well, there are at least two patchsets we're actively discussing
> reverting, so I think this should wait till those decisions are resolved.

OK, but that may well mean we don't get this done before beta1, which
I think is a bummer, but oh well.

There are a lot more than 2 patchsets that are busted at the moment,
unfortunately, but I assume you are referring to "snapshot too old"
and "Use Foreign Key relationships to infer multi-column join
selectivity".

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> OK, I committed this.  Barring objections, I'll go ahead and pgindent
> the whole tree tomorrow.  If we're going to revert anything big then
> we might want to hold off, but otherwise I think its better to get
> this done sooner rather than later.

Well, there are at least two patchsets we're actively discussing
reverting, so I think this should wait till those decisions are resolved.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 9:40 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> 1. Is pgindent supposed to touch DATA() lines?  Because it does.
>
> It always has.
>
>> 2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?
>
> Probably because there are no variables, parameters, or fields declared
> with that typedef, so it doesn't get into the debugger symbol table of
> any .o file.  Grep says that the single use of the struct doesn't use
> the typedef; it's
> const struct CustomPathMethods *methods;
> So you'd need to change that, or else tweak some code somewhere to have
> a variable explicitly declared using the typedef.

Ah.  It's a minor issue, so probably not worth worrying about.

>> - In src/backend/executor/execParallel.c, it dodges two cases where
>> pgindent does stupid things with offsetof.
>
> Well, it's also pretty stupid about sizeof, or casts generally, so
> I'm not really convinced you need to get exercised about these two
> places in particular.  But no objection to tweaking them if you
> want to.

OK, I committed this.  Barring objections, I'll go ahead and pgindent
the whole tree tomorrow.  If we're going to revert anything big then
we might want to hold off, but otherwise I think its better to get
this done sooner rather than later.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> 1. Is pgindent supposed to touch DATA() lines?  Because it does.

It always has.

> 2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?

Probably because there are no variables, parameters, or fields declared
with that typedef, so it doesn't get into the debugger symbol table of
any .o file.  Grep says that the single use of the struct doesn't use
the typedef; it's
const struct CustomPathMethods *methods;
So you'd need to change that, or else tweak some code somewhere to have
a variable explicitly declared using the typedef.

> - In src/backend/executor/execParallel.c, it dodges two cases where
> pgindent does stupid things with offsetof.

Well, it's also pretty stupid about sizeof, or casts generally, so
I'm not really convinced you need to get exercised about these two
places in particular.  But no objection to tweaking them if you
want to.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgindent fixups

[Andrew Dunstan]

On 05/02/2016 10:56 PM, Robert Haas wrote:

I spent some time going through the output of a trial pgindent run
today.  Some questions/problems:

1. Is pgindent supposed to touch DATA() lines?  Because it does.



Apart from being detabbed/entabbed, no. pgindent protects (or tries to 
protect) DATA and CATALOG lines by enclosng them in comments which it 
later removes.





2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?



Because it's not used anywhere. typdefs get into the list by being used 
and thus having a typedef symbol in the compiled binary. Just creating a 
typedef isn't enough. This has happened before.


You can get some insight into how the typedefs list is generated by 
looking here: 



cheers

andrew




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pgindent fixups

[Robert Haas]

I spent some time going through the output of a trial pgindent run
today.  Some questions/problems:

1. Is pgindent supposed to touch DATA() lines?  Because it does.

2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?

I'm attaching a patch that fixes up a few other problems that I found,
which I'll commit if there are not objections.  In detail:

- In contrib/pageinspect/heapfuncs.c, it separates the declaration of
bits_len from the initialization to avoid an awkward line-wrap.

- In src/backend/executor/execParallel.c, it dodges two cases where
pgindent does stupid things with offsetof.  Apparently, pgindent
thinks that you should write "offsetof(a, b) +c" rather than
"offsetof(a, b) + c".  In one case, I talked it out of it by putting
the + at the end of the first line rather than the start of the
continuation line.  The other statement was all on one line so I
changed it to say "c + offsetof(a, b)" instead.

- In nodeAgg.c, to_ts_any.c, and tsvector_op.c, I moved end-of-line
comments to their own separate lines, because they were getting broken
up into multiple lines in ways that seemed awkward.  In tsginidx.c, I
left a similar comment inline but fiddled the whitespace and comment
text to avoid getting a line break in mid-comment.

- In spell.c, I added  markers around a comment to prevent
pgindent from messing with the whitespace (entab still adjusts it, but
that should look the same if you have your tab stops set right).

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


pgindent-cleanup.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers