wrong comments in ClassifyUtilityCommandAsReadOnly

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

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

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

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

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