Re: Portal->commandTag as an enum

2020-03-16 Thread Alvaro Herrera
On 2020-Mar-04, Mark Dilger wrote: > > > > On Mar 2, 2020, at 1:57 PM, Alvaro Herrera wrote: > > > > I pushed it now. > > Thanks again! While rebasing some other work on top, I noticed one of your > comments is out of date: > > --- a/src/include/tcop/cmdtaglist.h > +++

Re: Portal->commandTag as an enum

2020-03-04 Thread Mark Dilger
> On Mar 2, 2020, at 1:57 PM, Alvaro Herrera wrote: > > I pushed it now. Thanks again! While rebasing some other work on top, I noticed one of your comments is out of date: --- a/src/include/tcop/cmdtaglist.h +++ b/src/include/tcop/cmdtaglist.h @@ -23,7 +23,7 @@ * textual name, so that

Re: Portal->commandTag as an enum

2020-03-02 Thread Mark Dilger
> On Mar 2, 2020, at 1:57 PM, Alvaro Herrera wrote: > > On 2020-Mar-02, Mark Dilger wrote: > >> I think it is more natural to change event trigger code to reason >> natively about CommandTags rather than continuing to reason about >> strings. The conversion back-and-forth between the enum

Re: Portal->commandTag as an enum

2020-03-02 Thread Alvaro Herrera
On 2020-Mar-02, Mark Dilger wrote: > I think it is more natural to change event trigger code to reason > natively about CommandTags rather than continuing to reason about > strings. The conversion back-and-forth between the enum and the > string representation serves no useful purpose that I can

Re: Portal->commandTag as an enum

2020-03-02 Thread Mark Dilger
> On Mar 2, 2020, at 9:08 AM, Alvaro Herrera wrote: > > On 2020-Mar-02, Alvaro Herrera wrote: > >> Here's the patch I propose for commit. I also rewrote the commit >> message. > > BTW I wonder if we should really change the definition of > EventTriggerData. ISTM that it would be sensible

Re: Portal->commandTag as an enum

2020-03-02 Thread Alvaro Herrera
On 2020-Mar-02, Alvaro Herrera wrote: > Here's the patch I propose for commit. I also rewrote the commit > message. BTW I wonder if we should really change the definition of EventTriggerData. ISTM that it would be sensible to keep it the same for now ... -- Álvaro Herrera

Re: Portal->commandTag as an enum

2020-03-02 Thread Alvaro Herrera
Here's the patch I propose for commit. I also rewrote the commit message. There are further refinements that can be done, but they don't need to be in the first patch. Notably, the event trigger code can surely do a lot better now by translating the tag list to a bitmapset earlier. -- Álvaro

Re: Portal->commandTag as an enum

2020-03-02 Thread Alvaro Herrera
On 2020-Mar-02, Mark Dilger wrote: > > I don't > > understand your point of pg_stats_sql having to deal with this in a > > particular way. How is that patch obtaining the command tags? I would > > hope it calls GetCommandTagName() rather than call CommandEnd, but maybe > > I misunderstand where

Re: Portal->commandTag as an enum

2020-03-02 Thread Mark Dilger
> On Mar 2, 2020, at 8:12 AM, Alvaro Herrera wrote: > > On 2020-Feb-29, Mark Dilger wrote: > >> You may want to think about the embedding of InvalidOid into the EndCommand >> output differently from how you think about the embedding of the rowcount >> into the EndCommand output, but my

Re: Portal->commandTag as an enum

2020-03-02 Thread Alvaro Herrera
On 2020-Feb-29, Mark Dilger wrote: > You may want to think about the embedding of InvalidOid into the EndCommand > output differently from how you think about the embedding of the rowcount > into the EndCommand output, but my preference is to treat these issues the > same and make a strong

Re: Portal->commandTag as an enum

2020-02-29 Thread Mark Dilger
> On Feb 28, 2020, at 5:42 PM, Tom Lane wrote: > > Mark Dilger writes: >>> On Feb 28, 2020, at 3:05 PM, Tom Lane wrote: >>> Is there a way to drop that logic altogether by making the tagname string >>> be "INSERT 0" for the INSERT case? Or would the zero bleed into other >>> places where

Re: Portal->commandTag as an enum

2020-02-28 Thread Tom Lane
Mark Dilger writes: >> On Feb 28, 2020, at 3:05 PM, Tom Lane wrote: >> Is there a way to drop that logic altogether by making the tagname string >> be "INSERT 0" for the INSERT case? Or would the zero bleed into other >> places where we don't want it? > In general, I don't think we want to

Re: Portal->commandTag as an enum

2020-02-28 Thread Mark Dilger
> On Feb 28, 2020, at 3:05 PM, Tom Lane wrote: > > Alvaro Herrera writes: >> I just realized that we could rename command_tag_display_last_oid() to >> something like command_tag_print_a_useless_zero_for_historical_reasons() >> and nothing would be lost. > > Is there a way to drop that

Re: Portal->commandTag as an enum

2020-02-28 Thread Alvaro Herrera
On 2020-Feb-28, Tom Lane wrote: > Alvaro Herrera writes: > > I just realized that we could rename command_tag_display_last_oid() to > > something like command_tag_print_a_useless_zero_for_historical_reasons() > > and nothing would be lost. > > Is there a way to drop that logic altogether by

Re: Portal->commandTag as an enum

2020-02-28 Thread Tom Lane
Alvaro Herrera writes: > I just realized that we could rename command_tag_display_last_oid() to > something like command_tag_print_a_useless_zero_for_historical_reasons() > and nothing would be lost. Is there a way to drop that logic altogether by making the tagname string be "INSERT 0" for the

Re: Portal->commandTag as an enum

2020-02-28 Thread Alvaro Herrera
I just realized that we could rename command_tag_display_last_oid() to something like command_tag_print_a_useless_zero_for_historical_reasons() and nothing would be lost. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training &

Re: Portal->commandTag as an enum

2020-02-28 Thread Alvaro Herrera
On 2020-Feb-28, Alvaro Herrera wrote: > On 2020-Feb-21, John Naylor wrote: > > > Thinking about this some more, would it be possible to treat these > > like we do parser/kwlist.h? Something like this: > > > > commandtag_list.h: > > PG_COMMANDTAG(ALTER_ACCESS_METHOD, "ALTER ACCESS METHOD", true,

Re: Portal->commandTag as an enum

2020-02-28 Thread Alvaro Herrera
On 2020-Feb-21, John Naylor wrote: > Thinking about this some more, would it be possible to treat these > like we do parser/kwlist.h? Something like this: > > commandtag_list.h: > PG_COMMANDTAG(ALTER_ACCESS_METHOD, "ALTER ACCESS METHOD", true, false, > false, false) > ... I liked this idea, so

Re: Portal->commandTag as an enum

2020-02-21 Thread John Naylor
Thinking about this some more, would it be possible to treat these like we do parser/kwlist.h? Something like this: commandtag_list.h: PG_COMMANDTAG(ALTER_ACCESS_METHOD, "ALTER ACCESS METHOD", true, false, false, false) ... then, just: #define PG_COMMANDTAG(taglabel, tagname, event_trigger,

Re: Portal->commandTag as an enum

2020-02-20 Thread John Naylor
On Thu, Feb 20, 2020 at 5:16 AM Mark Dilger wrote: > > > On Feb 19, 2020, at 3:31 AM, John Naylor > > wrote: > > thought it through. For one advantage, I think it might be nicer to > > have indexing.dat and toasting.dat instead of having to dig the info > > out of C macros. This was evident

Re: Portal->commandTag as an enum

2020-02-19 Thread John Naylor
Hi Mark, On Wed, Feb 19, 2020 at 10:40 AM Mark Dilger wrote: > This would only make sense to me if the string held in $_ had already been > checked for safety, but Catalog.pm does very little to verify that the string > is safe to eval. The assumption here, so far as I can infer, is that we

Re: Portal->commandTag as an enum

2020-02-11 Thread Mark Dilger
> On Feb 11, 2020, at 1:02 PM, Alvaro Herrera wrote: > > On 2020-Feb-11, Mark Dilger wrote: > >>> No thanks. >> >> I’m not sure which option you are voting for: >> >> (Option #1) Have the perl script generate the .c and .h file from a .dat file >> >> (Option #2) Have the perl script

Re: Portal->commandTag as an enum

2020-02-11 Thread Alvaro Herrera
On 2020-Feb-11, Mark Dilger wrote: > > No thanks. > > I’m not sure which option you are voting for: > > (Option #1) Have the perl script generate the .c and .h file from a .dat file > > (Option #2) Have the perl script validate but not generate the .c and .h files > > (Option #3) Have no perl

Re: Portal->commandTag as an enum

2020-02-11 Thread Mark Dilger
> On Feb 11, 2020, at 12:50 PM, Alvaro Herrera wrote: > > On 2020-Feb-11, Mark Dilger wrote: > >> I thought about generating the files rather than merely checking them. >> I could see arguments both ways. I wasn’t sure whether there would be >> broad support for having yet another perl

Re: Portal->commandTag as an enum

2020-02-11 Thread Alvaro Herrera
On 2020-Feb-11, Mark Dilger wrote: > I thought about generating the files rather than merely checking them. > I could see arguments both ways. I wasn’t sure whether there would be > broad support for having yet another perl script generating source > files, nor for the maintenance burden of

Re: Portal->commandTag as an enum

2020-02-11 Thread Mark Dilger
> On Feb 11, 2020, at 11:09 AM, Alvaro Herrera wrote: > > On 2020-Feb-07, Mark Dilger wrote: > >> Andres, >> >> The previous patch set seemed to cause confusion, having separated >> changes into multiple files. The latest patch, heavily influenced by >> your review, is all in one file,

Re: Portal->commandTag as an enum

2020-02-11 Thread Alvaro Herrera
On 2020-Feb-07, Mark Dilger wrote: > Andres, > > The previous patch set seemed to cause confusion, having separated > changes into multiple files. The latest patch, heavily influenced by > your review, is all in one file, attached. Cool stuff. I think is a little confused about what is source

Re: Portal->commandTag as an enum

2020-02-04 Thread Mark Dilger
> On Feb 4, 2020, at 7:34 PM, Andres Freund wrote: > > Hi, Thanks for reviewing! I am pretty much in agreement with your comments, below. > On 2020-02-04 18:18:52 -0800, Mark Dilger wrote: >> In master, a number of functions pass a char *completionTag argument (really >> a char

Re: Portal->commandTag as an enum

2020-02-04 Thread Andres Freund
Hi, On 2020-02-04 18:18:52 -0800, Mark Dilger wrote: > In master, a number of functions pass a char *completionTag argument (really > a char completionTag[COMPLETION_TAG_BUFSIZE]) which gets filled in with the > string to return to the client from EndCommand. I have removed that kind of >

Re: Portal->commandTag as an enum

2020-02-03 Thread Mark Dilger
> On Feb 2, 2020, at 6:14 PM, Tom Lane wrote: > > Mark Dilger writes: >> I put the CommandTag enum in src/common because there wasn’t any reason >> not to do so. It seems plausible that frontend test frameworks might want >> access to this enum. > Thanks for looking! > Au contraire,

Re: Portal->commandTag as an enum

2020-02-02 Thread Tom Lane
Mark Dilger writes: > I put the CommandTag enum in src/common because there wasn’t any reason > not to do so. It seems plausible that frontend test frameworks might want > access to this enum. Au contraire, that's an absolutely fundamental mistake. There is zero chance of this enum holding