Re: Should pg_current_wal_location() become pg_current_wal_lsn()

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)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.
> b) Generally, things read less nicely and look more complicated.

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

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

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%' and attrelid<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.

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.

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

#3: Rename the functions but not the columns.

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

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

#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.
With #4, we have a rule that function names use "location" while column
names use "lsn", which is a bit odd but not terrible.
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.

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.

regards, tom lane

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.