Обсуждение: suggest to rename enable_incrementalsort
I suggest to rename enable_incrementalsort to enable_incremental_sort. This is obviously more readable and also how we have named recently added multiword planner parameters. See attached patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > I suggest to rename enable_incrementalsort to enable_incremental_sort. > This is obviously more readable and also how we have named recently > added multiword planner parameters. > > See attached patch. +1, this is a way better name (and patch LGTM on REL_13_STABLE).
On Sun, Jun 21, 2020 at 09:05:32AM +0200, Julien Rouhaud wrote: >On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut ><peter.eisentraut@2ndquadrant.com> wrote: >> >> I suggest to rename enable_incrementalsort to enable_incremental_sort. >> This is obviously more readable and also how we have named recently >> added multiword planner parameters. >> >> See attached patch. > >+1, this is a way better name (and patch LGTM on REL_13_STABLE). > The reason why I kept the single-word variant is consistency with other GUCs that affect planning, like enable_indexscan, enable_hashjoin and many others. That being said, I'm not particularly attached this choice, so if you think this is better I'm OK with it. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, 21 Jun 2020 at 23:22, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > > On Sun, Jun 21, 2020 at 09:05:32AM +0200, Julien Rouhaud wrote: > >On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut > ><peter.eisentraut@2ndquadrant.com> wrote: > >> > >> I suggest to rename enable_incrementalsort to enable_incremental_sort. > >> This is obviously more readable and also how we have named recently > >> added multiword planner parameters. > >> > >> See attached patch. > > > >+1, this is a way better name (and patch LGTM on REL_13_STABLE). > > > > The reason why I kept the single-word variant is consistency with other > GUCs that affect planning, like enable_indexscan, enable_hashjoin and > many others. Looking at the other enable_* GUCs, for all the ones that aim to disable a certain executor node type, with the exception of enable_hashagg and enable_bitmapscan, they're all pretty consistent in naming the GUC after the executor node's .c file: enable_bitmapscan nodeBitmapHeapscan.c enable_gathermerge nodeGatherMerge.c enable_hashagg nodeAgg.c enable_hashjoin nodeHashjoin.c enable_incrementalsort nodeIncrementalSort.c enable_indexonlyscan nodeIndexonlyscan.c enable_indexscan nodeIndexscan.c enable_material nodeMaterial.c enable_mergejoin nodeMergejoin.c enable_nestloop nodeNestloop.c enable_parallel_append nodeAppend.c enable_parallel_hash nodeHash.c enable_partition_pruning enable_partitionwise_aggregate enable_partitionwise_join enable_seqscan nodeSeqscan.c enable_sort nodeSort.c enable_tidscan nodeTidscan.c enable_partition_pruning, enable_partitionwise_aggregate, enable_partitionwise_join are the odd ones out here as they're not really related to a specific node type. Going by that, it does seem the current name for enable_incrementalsort is consistent with the majority. Naming it enable_incremental_sort looks like it would be more suited if the feature had been added by overloading nodeSort.c. In that regard, it would be similar to enable_parallel_append and enable_parallel_hash, where the middle word becomes a modifier. David
On Mon, Jun 22, 2020 at 4:48 AM David Rowley <dgrowleyml@gmail.com> wrote: > > On Sun, 21 Jun 2020 at 23:22, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > > > > On Sun, Jun 21, 2020 at 09:05:32AM +0200, Julien Rouhaud wrote: > > >On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut > > ><peter.eisentraut@2ndquadrant.com> wrote: > > >> > > >> I suggest to rename enable_incrementalsort to enable_incremental_sort. > > >> This is obviously more readable and also how we have named recently > > >> added multiword planner parameters. > > >> > > >> See attached patch. > > > > > >+1, this is a way better name (and patch LGTM on REL_13_STABLE). > > > > > > > The reason why I kept the single-word variant is consistency with other > > GUCs that affect planning, like enable_indexscan, enable_hashjoin and > > many others. > > Looking at the other enable_* GUCs, for all the ones that aim to > disable a certain executor node type, with the exception of > enable_hashagg and enable_bitmapscan, they're all pretty consistent in > naming the GUC after the executor node's .c file: > > enable_bitmapscan nodeBitmapHeapscan.c > enable_gathermerge nodeGatherMerge.c > enable_hashagg nodeAgg.c > enable_hashjoin nodeHashjoin.c > enable_incrementalsort nodeIncrementalSort.c > enable_indexonlyscan nodeIndexonlyscan.c > enable_indexscan nodeIndexscan.c > enable_material nodeMaterial.c > enable_mergejoin nodeMergejoin.c > enable_nestloop nodeNestloop.c > enable_parallel_append nodeAppend.c > enable_parallel_hash nodeHash.c > enable_partition_pruning > enable_partitionwise_aggregate > enable_partitionwise_join > enable_seqscan nodeSeqscan.c > enable_sort nodeSort.c > enable_tidscan nodeTidscan.c > > enable_partition_pruning, enable_partitionwise_aggregate, > enable_partitionwise_join are the odd ones out here as they're not > really related to a specific node type. Thanks for the list. To me it's more of a question about readability than consistency. enable_mergejoin, enable_hashjoin for example are readable even without separating words merge_join or hash_join (many times I have typed enable_hash_join and cursed :); but that was before autocomplete was available). But enable_partitionwiseaggregate does not look much different from enable_abracadabra :). Looking from that angle, enable_incremental_sort is better than enable_incrementalsort. We could have named enable_indexonlyscan as enable_index_only_scan for better readability. -- Best Wishes, Ashutosh Bapat
On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > The reason why I kept the single-word variant is consistency with other > GUCs that affect planning, like enable_indexscan, enable_hashjoin and > many others. Right, so that makes sense, but from a larger point of view, how much sense does it actually make? I mean, I get the argument from tradition and from internal naming consistency, but from a user perspective, why does it makes sense for there to be underscores between some of the words and not others? I think it just feels random, like someone is charging us $1 per underscore so we're economizing. So I'm +1 for changing this, and I'd definitely be +1 for renaming the others if they weren't released already, and at least +0.5 for it anyhow. It's bad enough that our source code has names_like_this and NamesLikeThis and namesLikeThis; when we also start adding names_likethis and NamesLike_this and maybe NaMeS___LiKeTh_is, I kind of lose my mind. And avoiding that sort of thing in user-facing stuff seems even more important. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jun 22, 2020 at 10:16:54AM -0400, Robert Haas wrote: >On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra ><tomas.vondra@2ndquadrant.com> wrote: >> The reason why I kept the single-word variant is consistency with other >> GUCs that affect planning, like enable_indexscan, enable_hashjoin and >> many others. > >Right, so that makes sense, but from a larger point of view, how much >sense does it actually make? I mean, I get the argument from tradition >and from internal naming consistency, but from a user perspective, why >does it makes sense for there to be underscores between some of the >words and not others? I think it just feels random, like someone is >charging us $1 per underscore so we're economizing. > Sure. I'm not particularly attached to the current GUC, I've only tried to explain that the naming was not entirely random. I agree having an extra _ in the name would make it more readable. >So I'm +1 for changing this, and I'd definitely be +1 for renaming the >others if they weren't released already, and at least +0.5 for it >anyhow. It's bad enough that our source code has names_like_this and >NamesLikeThis and namesLikeThis; when we also start adding >names_likethis and NamesLike_this and maybe NaMeS___LiKeTh_is, I kind >of lose my mind. And avoiding that sort of thing in user-facing stuff >seems even more important. > OK, challenge accepted. $100 to the first person who commits a patch with a variable NaMeS___LiKeTh_is. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> The reason why I kept the single-word variant is consistency with other
>> GUCs that affect planning, like enable_indexscan, enable_hashjoin and
>> many others.
> Right, so that makes sense, but from a larger point of view, how much
> sense does it actually make?
Maybe I'm just used to the names, but I find that things like
"enable_seqscan" and "enable_nestloop" are pretty readable.
Once they get longer, though, not so much. So I agree with
renaming enable_incrementalsort.
> So I'm +1 for changing this, and I'd definitely be +1 for renaming the
> others if they weren't released already, and at least +0.5 for it
> anyhow.
Nah. Those names are way too well entrenched. Besides which, if
we open them up for reconsideration, there's going to be a lot of
bikeshedding done. Should "enable_seqscan" become "enable_seq_scan",
or "enable_sequential_scan", or maybe "enable_scan_sequential"?
Why doesn't "enable_nestloop" contain the word "join"? Etc etc.
(I do have to wonder if maybe this one should be enable_sort_incremental.)
regards, tom lane
On Mon, Jun 22, 2020 at 10:31 AM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > OK, challenge accepted. $100 to the first person who commits a patch > with a variable NaMeS___LiKeTh_is. :-) Well, that was hyperbole, but people have proposed some pretty wacky schemes, and a few of those have ended up in the tree. For example we have AtEOXact_PgStat and its close friend AtEOXact_on_commit_actions, for instance, or out_gistxlogDelete, or IncrementVarSublevelsUp_rtable, or convert_EXISTS_sublink_to_join. I confess haven't managed to find any plausible examples of underscores in the middle of a word yet, and we only have a handful of examples of double-underscore and none with triple-underscore, but we've got nearly every combination of lower-case words, upper-case words, initial-capital words, underscores separating words or not, and words abbreviated or not, and it's not hard to find cases where several different styles are used in the same identifier. This isn't the end of the world or anything, but I think we would be better off if we tried to do less of it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jun 22, 2020 at 10:41:17AM -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra > > <tomas.vondra@2ndquadrant.com> wrote: > >> The reason why I kept the single-word variant is consistency with other > >> GUCs that affect planning, like enable_indexscan, enable_hashjoin and > >> many others. > > > Right, so that makes sense, but from a larger point of view, how much > > sense does it actually make? > > Maybe I'm just used to the names, but I find that things like > "enable_seqscan" and "enable_nestloop" are pretty readable. > Once they get longer, though, not so much. So I agree with > renaming enable_incrementalsort. I think the big problem is that, without the extra underscore, it reads as increment-alsort. ;-) -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Bruce Momjian <bruce@momjian.us> writes:
> On Mon, Jun 22, 2020 at 10:41:17AM -0400, Tom Lane wrote:
>> Maybe I'm just used to the names, but I find that things like
>> "enable_seqscan" and "enable_nestloop" are pretty readable.
>> Once they get longer, though, not so much. So I agree with
>> renaming enable_incrementalsort.
> I think the big problem is that, without the extra underscore, it reads
> as increment-alsort. ;-)
Yeah, the longer the name gets, the harder it is to see where the
word boundaries are.
regards, tom lane
On Mon, Jun 22, 2020 at 11:22 AM Bruce Momjian <bruce@momjian.us> wrote: > I think the big problem is that, without the extra underscore, it reads > as increment-alsort. ;-) I know you're joking, but I think there's a serious issue here. We often both omit word separators and also abbreviate, and I doubt that the meaning is always obvious to people whose first language is Japanese or Russian or something. The only human language other than English in which I have any competence at all is Spanish, and if somebody speaks Spanish to me the way that it's explained in a textbook, I can understand it fairly well, especially if we're talking about the kinds of topics that textbooks discuss rather than technical stuff. But as soon as you start to use abbreviations or idioms, you're going to lose me. Without a doubt, the best solution to this problem would be for me to have better Spanish, but in the absence of that, on those occasions when I need to communicate in Spanish, I sure do like it when people are willing and able to make that as easy for me as they can. I suspect other people have similar experiences. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I think the change makes a lot of sense. The only reason I had it as enable_incrementalsort in the first place was trying to broadly following the existing GUC names, but as has already been pointed out, there's a lot of variation there, and my version of the patch already changed it to be more readable (at one point it was enable_incsort...which is short...but does not have an obvious meaning). I've attached a patch to make the change, though if people are interested in Tom's suggestion of enable_sort_incremental I could switch to that. James
Вложения
On 2020-07-02 17:25, James Coleman wrote: > I think the change makes a lot of sense. The only reason I had it as > enable_incrementalsort in the first place was trying to broadly > following the existing GUC names, but as has already been pointed out, > there's a lot of variation there, and my version of the patch already > changed it to be more readable (at one point it was > enable_incsort...which is short...but does not have an obvious > meaning). > > I've attached a patch to make the change, committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services