Horton

Thread

Commits

  1. Rename enable_incrementalsort for clarity

  1. suggest to rename enable_incrementalsort

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-21T06:26:34Z

    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

  2. Re: suggest to rename enable_incrementalsort

    Julien Rouhaud <rjuju123@gmail.com> — 2020-06-21T07:05:32Z

    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).

  3. Re: suggest to rename enable_incrementalsort

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-06-21T11:21:59Z

    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

  4. Re: suggest to rename enable_incrementalsort

    David Rowley <dgrowleyml@gmail.com> — 2020-06-21T23:18:24Z

    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

  5. Re: suggest to rename enable_incrementalsort

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2020-06-22T11:55:42Z

    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

  6. Re: suggest to rename enable_incrementalsort

    Robert Haas <robertmhaas@gmail.com> — 2020-06-22T14:16:54Z

    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

  7. Re: suggest to rename enable_incrementalsort

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-06-22T14:31:14Z

    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

  8. Re: suggest to rename enable_incrementalsort

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-22T14:41:17Z

    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

  9. Re: suggest to rename enable_incrementalsort

    Robert Haas <robertmhaas@gmail.com> — 2020-06-22T15:13:20Z

    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

  10. Re: suggest to rename enable_incrementalsort

    Bruce Momjian <bruce@momjian.us> — 2020-06-22T15:22:19Z

    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

  11. Re: suggest to rename enable_incrementalsort

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-22T16:16:34Z

    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

  12. Re: suggest to rename enable_incrementalsort

    Robert Haas <robertmhaas@gmail.com> — 2020-06-22T16:17:13Z

    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

  13. Re: suggest to rename enable_incrementalsort

    James Coleman <jtc331@gmail.com> — 2020-07-02T15:25:33Z

    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

  14. Re: suggest to rename enable_incrementalsort

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-07-05T10:20:21Z

    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