Re: [HACKERS] Should pg_current_wal_location() becomepg_current_wal_lsn() - Mailing list pgsql-hackers

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > On 5/1/17 08:10, David Rowley wrote:
> >> OK, so I've created a draft patch which does this.
>
> > After reading this patch, I see that
> > a) The scope of the compatibility break is expanded significantly beyond
> > what was already affected by the xlog->wal renaming.

Changing one additional view doesn't strike me as a significantly
expanded set.

> > b) Generally, things read less nicely and look more complicated.

I disagree.

> > So I still think we'd be better off leaving things the way they are.
>
> What I find in a bit of looking around is that
>
> 1. There are no functions named *lsn* except the support functions
> for the pg_lsn type.

Even so, that doesn't strike me as having been particularly intentional
or because it was "better" to use something else, especially given the
column naming.

> 2. There are half a dozen or so functions named *location* (the
> ones hit in this patch).  All of them existed in 9.6, but with
> names involving "xlog"; they're already renamed to involve "wal".

Right, so changing location -> lsn for those doesn't expand the
compatibility issue really.

> 3. We have these system columns named *lsn*:
>
> regression=# select attrelid::regclass, attname, atttypid::regtype from pg_attribute where attname like '%lsn%' and
attrelid<16384;
>            attrelid           |       attname       | atttypid
> ------------------------------+---------------------+----------
>  pg_stat_wal_receiver         | receive_start_lsn   | pg_lsn
>  pg_stat_wal_receiver         | received_lsn        | pg_lsn
>  pg_stat_wal_receiver         | latest_end_lsn      | pg_lsn
>  pg_replication_slots         | restart_lsn         | pg_lsn
>  pg_replication_slots         | confirmed_flush_lsn | pg_lsn
>  pg_replication_origin_status | remote_lsn          | pg_lsn
>  pg_replication_origin_status | local_lsn           | pg_lsn
>  pg_subscription_rel          | srsublsn            | pg_lsn
>  pg_stat_subscription         | received_lsn        | pg_lsn
>  pg_stat_subscription         | latest_end_lsn      | pg_lsn
> (10 rows)
>
> The first seven of those existed in 9.6.
>
> 4. We have these system columns named *location*:
>
> regression=# select attrelid::regclass, attname, atttypid::regtype from pg_attribute where attname like '%location%'
andattrelid<16384; 
>       attrelid       |     attname     | atttypid
> ---------------------+-----------------+----------
>  pg_stat_replication | sent_location   | pg_lsn
>  pg_stat_replication | write_location  | pg_lsn
>  pg_stat_replication | flush_location  | pg_lsn
>  pg_stat_replication | replay_location | pg_lsn
> (4 rows)
>
> All of those existed in 9.6.  The patch proposes to rename them to *lsn.

Right, four column names in one view- the one view which is different
from all of the other views that exist.

> So it seems like we do not have such a big problem with function names:
> the relevant functions all use "location" in their names, and we could
> just keep doing that.  The problem that we've got is inconsistency in
> table/view column names.  However, there is no way to fix it without
> adding a new dollop of compatibility break on top of what we've done
> already.
>
> For completeness, I think the available alternatives are:
>
> #1: Leave these names as they stand.

-1

> #2: Rename all these functions and columns to "lsn", as in this patch.

+1

> #3: Rename the functions but not the columns.

-1

> #4: Leave the function names alone, standardize on "lsn" in column names
> (hence, touching pg_stat_replication only).

-1

> #5: Standardize on "location" not "lsn" (hence, leaving the function
> names alone, and touching pg_stat_wal_receiver, pg_replication_slots,
> and pg_replication_origin_status, as well as the new-in-v10-anyway
> pg_subscription_rel and pg_stat_subscription).

Ugh, that seems even worse, and expands the compatibility breakage, -5.

> #3 has the advantage of not breaking anything we didn't break already,
> but it isn't accomplishing very much in terms of improving consistency.

Agreed.

> With #4, we have a rule that function names use "location" while column
> names use "lsn", which is a bit odd but not terrible.

Given that we're changing the function names already, I really don't see
why this makes any sense.  Perhaps it's just me, but 'location' is much
more of a vague term than 'lsn' and if we're talking about an 'lsn' then
that's what we should be using.

> I think #5 would best serve Peter's point about readability, because
> I agree that "lsn" isn't doing us any favors in that direction.

I disagree that 'lsn' is worse than 'location'- at least for my part,
it's much more precise and clear about what we're talking about.
"location" could be where a file exists on the disk, where in the world
we are, where we are in the heap, where we are when stepping through an
index, etc.  With the work on adding progress information for things
like VACUUM, and I think there was something about tracking progress of
copying a table for logical replication, it certainly seems likely that
we may have cause to think about the point we are in the heap or in an
index.

> So this boils down to whether we are willing to touch any of these
> column names in order to improve consistency.  I think it might be
> worth doing, but there's no doubt that we're adding more compatibility
> pain if we do anything but #1 or #3.

#2 strikes me as the best option, though that's probably not much of a
surprise to anyone whose been following my comments on this thread.

> PS: There are some other changes in David's patch, such as
> s/position/location/ in some text, that I think we should do in any
> case.  But the first decision has to be about the view column names.

Agreed.

Thanks!

Stephen