Re: suggest to rename enable_incrementalsort

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