Horton

Thread

Commits

  1. Fix incorrect source filename references

  1. wrong comments in ClassifyUtilityCommandAsReadOnly

    jian he <jian.universality@gmail.com> — 2024-12-21T04:05:12Z

    hi.

/*
 * Determine the degree to which a utility command is read only.
 *
 * Note the definitions of the relevant flags in src/include/utility/tcop.h.
 */
static int
ClassifyUtilityCommandAsReadOnly(Node *parsetree)

Is the comment wrong?

it should be
" * Note the definitions of the relevant flags in src/include/tcop/utility.h."

  2. Re: wrong comments in ClassifyUtilityCommandAsReadOnly

    David Rowley <dgrowleyml@gmail.com> — 2024-12-23T02:43:09Z

    On Sat, 21 Dec 2024 at 17:06, jian he <jian.universality@gmail.com> wrote:
>  * Note the definitions of the relevant flags in src/include/utility/tcop.h.
>  */
> static int
> ClassifyUtilityCommandAsReadOnly(Node *parsetree)
>
> Is the comment wrong?
>
> it should be
> " * Note the definitions of the relevant flags in src/include/tcop/utility.h."

Yeah. There is no tcop.h.

 I used the attached (not exactly very pretty) script to find a few more.

git grep -h -E "\Wsrc/[\w_/]*" -- '*.[ch]' | tr -d '"' | tr -d \'\" |
xargs -n 1 ./fileexists.sh | grep "does not"

There are plenty of places that reference files not starting with
"src/". It might be good to verify there's some subdirectory in the
source tree that match those, however, I didn't think of a creative
way to identify those ones.

David

  3. Re: wrong comments in ClassifyUtilityCommandAsReadOnly

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-23T03:37:40Z

    David Rowley <dgrowleyml@gmail.com> writes:
> + *	  The "DefineAggregate" routine take the parse tree and pick out the
> + *	  appropriate arguments/flags, passing the results to
> + *	  "AggregateCreate" routine (src src/backend/catalog) that do the actual
> + *	  catalog-munging.  These routines also verify permission of the user to
> + *	  execute the command.

The grammar needs some help here.  Also, rather than simply remove
define.c's entire header comment, maybe we should write something
relevant about what it does?  Good catches otherwise.

			regards, tom lane

  4. Re: wrong comments in ClassifyUtilityCommandAsReadOnly

    David Rowley <dgrowleyml@gmail.com> — 2024-12-23T05:27:37Z

    On Mon, 23 Dec 2024 at 16:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > + *     The "DefineAggregate" routine take the parse tree and pick out the
> > + *     appropriate arguments/flags, passing the results to
> > + *     "AggregateCreate" routine (src src/backend/catalog) that do the actual
> > + *     catalog-munging.  These routines also verify permission of the user to
> > + *     execute the command.
>
> The grammar needs some help here.

Ah oops. I forgot to check that before posting.

> Also, rather than simply remove
> define.c's entire header comment, maybe we should write something
> relevant about what it does?  Good catches otherwise.

I didn't have any inspiration on what to write other than what's
already written on line 4.  Another reason I deleted that is that
since the file contains helper functions, I didn't want to write a new
comment based on what functions are there now as it may put someone
else off from adding new ones if the new one doesn't fit the comment.

I'm happy to take suggestions if you can think of something. In the
meantime, I've attached the patch with the aggregatecmds.c comment
fixes.

David

  5. Re: wrong comments in ClassifyUtilityCommandAsReadOnly

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-23T05:53:11Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Mon, 23 Dec 2024 at 16:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Also, rather than simply remove
>> define.c's entire header comment, maybe we should write something
>> relevant about what it does?  Good catches otherwise.

> I didn't have any inspiration on what to write other than what's
> already written on line 4.

Hmm ... fair enough, I don't have a tighter spec either.  It looks
like the current situation is my fault --- 71dc300a3 should have
thought harder about how to update this header comment.

> Another reason I deleted that is that
> since the file contains helper functions, I didn't want to write a new
> comment based on what functions are there now as it may put someone
> else off from adding new ones if the new one doesn't fit the comment.

Perhaps we could define it as "Support routines for dealing with
DefElem nodes".  You're right that maybe someone would want to
throw in something else, but would it really belong?  The file's
charter seems far narrower now than it once was.

			regards, tom lane

  6. Re: wrong comments in ClassifyUtilityCommandAsReadOnly

    David Rowley <dgrowleyml@gmail.com> — 2024-12-23T06:56:35Z

    On Mon, 23 Dec 2024 at 18:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > Another reason I deleted that is that
> > since the file contains helper functions, I didn't want to write a new
> > comment based on what functions are there now as it may put someone
> > else off from adding new ones if the new one doesn't fit the comment.
>
> Perhaps we could define it as "Support routines for dealing with
> DefElem nodes".  You're right that maybe someone would want to
> throw in something else, but would it really belong?  The file's
> charter seems far narrower now than it once was.

I felt that it was better to leave the scope a bit wider than that,
but I don't feel very strongly, so I've pushed it with your wording
suggestion.

Thanks

David