Running pgindent

Bruce Momjian wrote:

I want to run pgindent before the final release. Who has outstanding
patches they are sitting on that would be affected by this? Only places
where we have added non-conforming code would be affected.

Ack! Can you make at least three changes to pgindent before doing this?

1) for functions which look like
datetime *
abstime_datetime(int4 x)
{
...
do not put a tab (or extra spaces) between "datetime" and "*".

For the end of functions which look like
...
} /* abstime_datetime() */

do not put any tabs (or just one) between the "{" and the comment.

Also, some comments get wrapped to the next line, making some ugly sources;
perhaps pgindent should not touch comment lines at all? Or at least keep
things on the same line, messing with the tabbing only??

Bruce Momjian wrote:

I want to run pgindent before the final release. Who has outstanding
patches they are sitting on that would be affected by this? Only places
where we have added non-conforming code would be affected.

Ack! Can you make at least three changes to pgindent before doing this?

1) for functions which look like
datetime *
abstime_datetime(int4 x)
{
...
do not put a tab (or extra spaces) between "datetime" and "*".

For the end of functions which look like
...
} /* abstime_datetime() */

do not put any tabs (or just one) between the "{" and the comment.

Also, some comments get wrapped to the next line, making some ugly sources;
perhaps pgindent should not touch comment lines at all? Or at least keep
things on the same line, messing with the tabbing only??

Well, that is what indent does. I can try and work around it, but I am
afraid I will mess it up somehow. Right now, all the stuff is indented
consistently, and running it again will only make the changed stuff look
like the rest.

Making a change will change all the code, even the unchanged stuff.
Also, for comments, it will not change comments that are block-style.
See the indent manual page and the flags I use for help. Keep in mind I
use BSD indent, which does not have the bugs in GNU indent.

--
Bruce Momjian
maillist@candle.pha.pa.us

Bruce Momjian wrote:

I want to run pgindent before the final release. Who has outstanding
patches they are sitting on that would be affected by this? Only places
where we have added non-conforming code would be affected.

I'm changing bufmgr.c & elog.c to fix problems with smgrblind.

Vadim

Bruce Momjian wrote:

I want to run pgindent before the final release. Who has outstanding
patches they are sitting on that would be affected by this? Only places
where we have added non-conforming code would be affected.

I'm changing bufmgr.c & elog.c to fix problems with smgrblind.

Vadim

Let me know when you would like it done.

--
Bruce Momjian
maillist@candle.pha.pa.us

Bruce Momjian wrote:

Bruce Momjian wrote:

I want to run pgindent before the final release. Who has outstanding
patches they are sitting on that would be affected by this? Only places
where we have added non-conforming code would be affected.

Ack! Can you make at least three changes to pgindent before doing this?

1) for functions which look like
datetime *
abstime_datetime(int4 x)
{
...
do not put a tab (or extra spaces) between "datetime" and "*".

For the end of functions which look like
...
} /* abstime_datetime() */

do not put any tabs (or just one) between the "{" and the comment.

Also, some comments get wrapped to the next line, making some ugly sources;
perhaps pgindent should not touch comment lines at all? Or at least keep
things on the same line, messing with the tabbing only??

Well, that is what indent does. I can try and work around it, but I am
afraid I will mess it up somehow. Right now, all the stuff is indented
consistently, and running it again will only make the changed stuff look
like the rest.

Making a change will change all the code, even the unchanged stuff.
Also, for comments, it will not change comments that are block-style.
See the indent manual page and the flags I use for help. Keep in mind I
use BSD indent, which does not have the bugs in GNU indent.

Well, *&^*^#$! I wasted hours cleaning up some of what I considered damage from
the last pass through.
Would it be possible to pass back through (i.e. pipe indent output to a filter)
and fix at least points (1) and (2)? _That_ would be pretty easy to automate
since the heuristics can be pretty simple:

if (first char is nonwhitespace) && (next word is "*") && (next line starts with
word+paren)
then compress whitespace

else if (first char is "}") && (next word is "/*")
then compress whitespace

else
write line as-is

I'll write the perl if you would be willing to use it?

- Tom

Bruce Momjian wrote:

I want to run pgindent before the final release. Who has outstanding
patches they are sitting on that would be affected by this? Only places
where we have added non-conforming code would be affected.

I'm changing bufmgr.c & elog.c to fix problems with smgrblind.

Vadim

Let me know when you would like it done.

Got very close to the view permission override - just a vew hours
for me :-)

--
Bruce Momjian
maillist@candle.pha.pa.us

Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #

Bruce Momjian wrote:

I want to run pgindent before the final release. Who has outstanding
patches they are sitting on that would be affected by this? Only places
where we have added non-conforming code would be affected.

I'm changing bufmgr.c & elog.c to fix problems with smgrblind.

Vadim

Let me know when you would like it done.

Got very close to the view permission override - just a vew hours
for me :-)

Great. I was thinking about this, and I think it would be best if the
view permissions were checked as the view owner at view USE time, not
view CREATION time, that way if permissions on the base tables change,
the permissions are properly honored.

I think the range table idea is good, so there is an OWNER field on each
range table which defaults to the current user. As views are replaced
by base tables in the rewrite system, the owner can be changed to the
owner of the view. The issue is whether the range table entry will be
available in the executor for you to access that owner field. But at
this point, any fix for this would be great. People are asking for this
view permissions thing.

Also, this makes non-super-user created views even harder, because if
people can create their own views, they can change the owner field to
anyone they want to, but that is for later.

--
Bruce Momjian
maillist@candle.pha.pa.us

Bruce wrote:

Got very close to the view permission override - just a vew hours
for me :-)

Great. I was thinking about this, and I think it would be best if the
view permissions were checked as the view owner at view USE time, not
view CREATION time, that way if permissions on the base tables change,
the permissions are properly honored.

Did it that way as it looked better for me too. :-)

I think the range table idea is good, so there is an OWNER field on each
range table which defaults to the current user. As views are replaced
by base tables in the rewrite system, the owner can be changed to the
owner of the view. The issue is whether the range table entry will be
available in the executor for you to access that owner field. But at
this point, any fix for this would be great. People are asking for this
view permissions thing.

Done much easier. Appended a bool field aclSkip to
RangeTblEntry that defaults to false due to makenode(). When
the rewriting system finds a rule on select on the relation
level, with exactly one action, the actions command type is a
select which is instead (wow) it is a view. This time the
rewrite handler checks acl for the range table entries in the
rules action against the owner of the table the rule is on
(the view itself). On success it sets aclSkip to true.

Later the executor in ExecCheckPerms() just skips these
entries. Since one range table entry for the view itself is
left without the aclSkip set the executor checks if the
current user has access rights for the view. But it skips
those entries appended to the range table by the rewrite
handler accessing the real tables in question.

One little thing isn't covered then. User A creates a view on
a table he revoked from world and grants access to the view
only to user B. Now user B can create another view selecting
A's view and grant that to world and this would do it. But
since any user that can read a view could simply copy the
data into another table and grant that to world I don't see a
really security problem here. Granting access to user implies
IMHO you can trust that user.

Also, this makes non-super-user created views even harder, because if
people can create their own views, they can change the owner field to
anyone they want to, but that is for later.

Do they - hmmm that's not good. But there could be a way
round. Really for later but let's keep the solution in mind.

We add a bool to pg_class that let's the rewrite handler know
if he really should set the aclSkip defaulting to false. On
ownership changes this flag is reset to false and only the
owner or superusers might set it.

Until later, Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #

Bruce wrote:

Got very close to the view permission override - just a vew hours
for me :-)

Great. I was thinking about this, and I think it would be best if the
view permissions were checked as the view owner at view USE time, not
view CREATION time, that way if permissions on the base tables change,
the permissions are properly honored.

Did it that way as it looked better for me too. :-)

Good.

I think the range table idea is good, so there is an OWNER field on each
range table which defaults to the current user. As views are replaced
by base tables in the rewrite system, the owner can be changed to the
owner of the view. The issue is whether the range table entry will be
available in the executor for you to access that owner field. But at
this point, any fix for this would be great. People are asking for this
view permissions thing.

Done much easier. Appended a bool field aclSkip to
RangeTblEntry that defaults to false due to makenode(). When
the rewriting system finds a rule on select on the relation
level, with exactly one action, the actions command type is a
select which is instead (wow) it is a view. This time the
rewrite handler checks acl for the range table entries in the
rules action against the owner of the table the rule is on
(the view itself). On success it sets aclSkip to true.

Later the executor in ExecCheckPerms() just skips these
entries. Since one range table entry for the view itself is
left without the aclSkip set the executor checks if the
current user has access rights for the view. But it skips
those entries appended to the range table by the rewrite
handler accessing the real tables in question.

One little thing isn't covered then. User A creates a view on
a table he revoked from world and grants access to the view
only to user B. Now user B can create another view selecting
A's view and grant that to world and this would do it. But
since any user that can read a view could simply copy the
data into another table and grant that to world I don't see a
really security problem here. Granting access to user implies
IMHO you can trust that user.

This sounds great to me. As you have added to RangeTableEntry, I assume
you also added the needed fields to copyfuncs.c/outfuncs.c/makefuncs.c
and anywhere else that needs it. I will add this to the developers FAQ.

Also, this makes non-super-user created views even harder, because if
people can create their own views, they can change the owner field to
anyone they want to, but that is for later.

Do they - hmmm that's not good. But there could be a way
round. Really for later but let's keep the solution in mind.

We add a bool to pg_class that let's the rewrite handler know
if he really should set the aclSkip defaulting to false. On
ownership changes this flag is reset to false and only the
owner or superusers might set it.

No, that is not what I was saying. If they can create views, the can
change pg_rewrite, and because we now take the view as the owners
permission granting, someone could change anything in pg_rewrite and
make it look like it is a view of someone else. They could change the
view text to look at pg_user, for example.

--
Bruce Momjian
maillist@candle.pha.pa.us

Bruce wrote:

This sounds great to me. As you have added to RangeTableEntry, I assume
you also added the needed fields to copyfuncs.c/outfuncs.c/makefuncs.c
and anywhere else that needs it. I will add this to the developers FAQ.

Only to copyfuncs.c now. Is it possible that they get
output/reread after deepRewriteQuery()?. I don't think so.
And since makenode() initializes the allocated memory to
nulls aclSkip defaults to false.

We add a bool to pg_class that let's the rewrite handler know
if he really should set the aclSkip defaulting to false. On
ownership changes this flag is reset to false and only the
owner or superusers might set it.

No, that is not what I was saying. If they can create views, the can
change pg_rewrite, and because we now take the view as the owners
permission granting, someone could change anything in pg_rewrite and
make it look like it is a view of someone else. They could change the
view text to look at pg_user, for example.

Instead of granting users access to pg_rewrite we should use
again some mechanism in rewriteDefine/ExecCheckPerms to let
users only create rules through the CREATE RULE command. Not
by INSERT INTO pg_rewrite. Then we can check during the
creation of a rule that the user is the owner of the relation
the rule is on. Totally safe then.

Until later, Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #

Bruce wrote:

This sounds great to me. As you have added to RangeTableEntry, I assume
you also added the needed fields to copyfuncs.c/outfuncs.c/makefuncs.c
and anywhere else that needs it. I will add this to the developers FAQ.

Only to copyfuncs.c now. Is it possible that they get
output/reread after deepRewriteQuery()?. I don't think so.
And since makenode() initializes the allocated memory to
nulls aclSkip defaults to false.

It doesn't matter whether we think it will never be used. If it says it
copies a structure, we have to make it work. Never know how it will be
used in the future. I went through all the copy/make/read/out files to
make sure every element of every structure was output, where possible.

I have added this to the developers FAQ, and it is on our web page.

We add a bool to pg_class that let's the rewrite handler know
if he really should set the aclSkip defaulting to false. On
ownership changes this flag is reset to false and only the
owner or superusers might set it.

No, that is not what I was saying. If they can create views, the can
change pg_rewrite, and because we now take the view as the owners
permission granting, someone could change anything in pg_rewrite and
make it look like it is a view of someone else. They could change the
view text to look at pg_user, for example.

Instead of granting users access to pg_rewrite we should use
again some mechanism in rewriteDefine/ExecCheckPerms to let
users only create rules through the CREATE RULE command. Not
by INSERT INTO pg_rewrite. Then we can check during the
creation of a rule that the user is the owner of the relation
the rule is on. Totally safe then.

Yep. Need this for pg_database too.

--
Bruce Momjian
maillist@candle.pha.pa.us

Only to copyfuncs.c now. Is it possible that they get
output/reread after deepRewriteQuery()?. I don't think so.
And since makenode() initializes the allocated memory to
nulls aclSkip defaults to false.

It doesn't matter whether we think it will never be used. If it says it
copies a structure, we have to make it work. Never know how it will be
used in the future. I went through all the copy/make/read/out files to
make sure every element of every structure was output, where possible.

Correct. I'll do it along with the flag in pg_class.

Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #

Only to copyfuncs.c now. Is it possible that they get
output/reread after deepRewriteQuery()?. I don't think so.
And since makenode() initializes the allocated memory to
nulls aclSkip defaults to false.

It doesn't matter whether we think it will never be used. If it says it
copies a structure, we have to make it work. Never know how it will be
used in the future. I went through all the copy/make/read/out files to
make sure every element of every structure was output, where possible.

Correct. I'll do it along with the flag in pg_class.

Can you remind me of the pg_class flag's purpose?

--
Bruce Momjian
maillist@candle.pha.pa.us

If I ever get to run pgindent. I will make these changes, Tom.

Well, *&^*^#$! I wasted hours cleaning up some of what I considered damage from
the last pass through.
Would it be possible to pass back through (i.e. pipe indent output to a filter)
and fix at least points (1) and (2)? _That_ would be pretty easy to automate
since the heuristics can be pretty simple:

if (first char is nonwhitespace) && (next word is "*") && (next line starts with
word+paren)
then compress whitespace

else if (first char is "}") && (next word is "/*")
then compress whitespace

else
write line as-is

I'll write the perl if you would be willing to use it?

- Tom

--
Bruce Momjian
maillist@candle.pha.pa.us

If I ever get to run pgindent. I will make these changes, Tom.

Ahhhhh. Thx. :)

- Tom

Well, *&^*^#$! I wasted hours cleaning up some of what I considered damage from
the last pass through.
Would it be possible to pass back through (i.e. pipe indent output to a filter)
and fix at least points (1) and (2)? _That_ would be pretty easy to automate
since the heuristics can be pretty simple:

if (first char is nonwhitespace) && (next word is "*") && (next line starts with
word+paren)
then compress whitespace

else if (first char is "}") && (next word is "/*")
then compress whitespace

else
write line as-is

I'll write the perl if you would be willing to use it?

OK, I have changed pgindent to reflect the new structure names, and have
made your requested changes. For #1, if the line began with an alpha,
and ends with a *, I remove tabs/spaces before the * and make it only
one space. Let me know how you like the changes.

I have not run it on the source yet because people are still holding
patches.

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

On Tue, 24 Feb 1998, Bruce Momjian wrote:

OK, I have changed pgindent to reflect the new structure names, and have
made your requested changes. For #1, if the line began with an alpha,
and ends with a *, I remove tabs/spaces before the * and make it only
one space. Let me know how you like the changes.

I have not run it on the source yet because people are still holding
patches.

Bruce, do you run pgindent on the java sources aswell?

If you do, a couple of points:

/* */ are as in C
/** same as /* but also marks the start of a comment (can be
html) that javadoc will place in documentation.
// Single line comment

You may want to check that it doesn't mangle the last two.

--
Peter T Mount petermount@earthling.net or pmount@maidast.demon.co.uk
Main Homepage: http://www.demon.co.uk/finder
Work Homepage: http://www.maidstone.gov.uk Work EMail: peter@maidstone.gov.uk

On Tue, 24 Feb 1998, Bruce Momjian wrote:

OK, I have changed pgindent to reflect the new structure names, and have
made your requested changes. For #1, if the line began with an alpha,
and ends with a *, I remove tabs/spaces before the * and make it only
one space. Let me know how you like the changes.

I have not run it on the source yet because people are still holding
patches.

Bruce, do you run pgindent on the java sources aswell?

If you do, a couple of points:

/* */ are as in C
/** same as /* but also marks the start of a comment (can be
html) that javadoc will place in documentation.
// Single line comment

You may want to check that it doesn't mangle the last two.

No, no run of C++ or Java.

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)