rename labels in heapam.c?

Andres Freund <andres@anarazel.de> writes:

For the umpteenth time I was annoyed by the names of labels in
heapam.c. It's really not useful to see a 'goto l1;' etc.

Yeah, those label names are uninformative as can be.

How about renaming l1 to retry_delete_locked, l2 to retry_update_locked,
l3 to retry_lock_tuple_locked etc? Especially with the subsidiary
functions for updates and locking, it's not always clear from context
where the goto jumps to.

Is it practical to get rid of the goto's altogether? If not,
renaming would be an improvement.

regards, tom lane

Hi,

On 2019-03-22 17:09:23 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

For the umpteenth time I was annoyed by the names of labels in
heapam.c. It's really not useful to see a 'goto l1;' etc.

Yeah, those label names are uninformative as can be.

How about renaming l1 to retry_delete_locked, l2 to retry_update_locked,
l3 to retry_lock_tuple_locked etc? Especially with the subsidiary
functions for updates and locking, it's not always clear from context
where the goto jumps to.

Is it practical to get rid of the goto's altogether? If not,
renaming would be an improvement.

I don't think it'd be easy. We could probably split
heap_{insert,delete,update} into sub-functions and then have the
toplevel function just loop over invocations of those, but that seems
like a pretty significant refactoring of the code. Since renaming the
labels isn't going to make that harder, I'm inclined to do that, rather
than wait for a refactoring that, while a good idea, isn't likely to
happen that soon.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2019-03-22 17:09:23 -0400, Tom Lane wrote:

Is it practical to get rid of the goto's altogether? If not,
renaming would be an improvement.

I don't think it'd be easy.

Fair enough. I just wanted to be sure we considered getting rid of
the pig before we put lipstick on it. I concur that it's not worth
major refactoring to do so.

regards, tom lane