Re: [Patch] pg_rewind: options to use restore_command fromrecovery.conf or command line

On 2020-06-08 09:14, Michael Paquier wrote:
> On Sun, Jun 07, 2020 at 10:05:03PM +0300, Alexander Korotkov wrote:
>> On Sat, Jun 6, 2020 at 8:49 PM Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> Why is fe_archive.c in src/common/ and not in src/fe_utils/?  It is 
>>> not
>>> "common" to frontend and backend.
>> 
>> Yep, this seems wrong to me.  I can propose following file rename.
>> 
>> src/common/fe_archive.c => src/fe_utils/archive.c
>> include/common/fe_archive.h => include/fe_utils/archive.h
> 
> Is it technically a problem though?  fe_archive.c is listed as a
> frontend-only file with OBJS_FRONTEND in src/common/Makefile.  Anyway,
> for this one I would not mind to do the move so please see the
> attached, but I am not completely sure either why src/fe_utils/ had
> better be chosen than src/common/.
> 

> 
> Perhaps we have more things that are frontend-only in src/common/ that
> could be moved to src/fe_utils/?  I am looking at restricted_token.c,
> fe_memutils.c, logging.c and file_utils.c here, but that would mean
> breaking a couple of declarations, something never free for plugin
> developers.
> 

I noticed this before in the thread [1], but it has been left unnoticed 
I guess:

"I just went through the both patches and realized that I cannot get 
into
semantics of splitting frontend code between common and fe_utils. This
applies only to 0002, where we introduce fe_archive.c. Should it be
placed into fe_utils alongside of the recent recovery_gen.c also used by
pg_rewind? This is a frontend-only code intended to be used by frontend
applications, so fe_utils feels like the right place, doesn't it? Just
tried to do so and everything went fine, so it seems that there is no
obstacles from the build system.

BTW, most of 'common' is a really common code with only four exceptions
like logging.c, which is frontend-only. Is it there for historical
reasons only or something else?"

Personally, I would prefer that everything in the 'common' was actually 
common. I also do not sure about moving an older code, because of 
possible backward compatibility breakage, but doing so for a newer one 
seems to be a good idea.

> 
>>> It actually defines functions that clash with functions in the 
>>> backend,
>>> so this seems doubly wrong.
>> 
>> Let's have frontend version of RestoreArchivedFile() renamed as well.
>> What about RestoreArchivedFileFrontend()?
> 
> -1 from me for the renaming, which was intentional to ease grepping
> with the backend counterpart.  We have other cases like that, say
> palloc(), fsync_fname(), etc.
> 

Do not like it either. Having the same naming and a guarantee that this 
code is always compiled separately looks more convenient for me.

[1] 
https://www.postgresql.org/message-id/784fa7dc-414b-9dc9-daae-138033db298c%40postgrespro.ru


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company