Re: walmethods.c/h are doing some strange things - Mailing list pgsql-hackers

At Fri, 2 Sep 2022 11:52:38 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> that type that can ever exist, and the pointer to that object is
> stored in a global variable managed by walmethods.c. So whereas in
> other cases we give you the object and then a way to get the
> corresponding set of callbacks, here we only give you the callbacks,
> and we therefore have to impose the artificial restriction that there
> can only ever be one object.

Makes sense to me.

> There is no real benefit in having the same variable in two different
> structs and having to access it via a callback when we could just put
> it into a common struct and access it directly. There's also a
> compression_algorithm() method which has exactly the same issue,
..
> though that is an overall property of the WalWriteMethod rather than a
> per-Walfile property. There's also a getlasterr callback which is
> basically just duplicate code across the two implementations; we could
> unify that code. There's also a global variable current_walfile_name[]
> in receivelog.c which only needs to exist because the file name is
> inconveniently hidden inside the WalWriteMethod abstraction layer; we
> can just make it visible.

Sounds sensible.

> Attached are a couple of hastily-written patches implementing this.

> patches is that the existing code isn't very clear about whether
> "Walfile" is supposed to be an abstraction for a pointer to the
> implementation-specific struct, or the struct itself. From looking at
> walmethods.h, you'd think it's a pointer to the struct, because we
> declare typedef void *Walfile. walmethods.c agrees, but receivelog.c
> takes a different view, declaring all of its variables as type
> "Walfile *". This doesn't cause a compiler error because void * is
> just as interchangeable with void ** as it is with DirectoryMethodFile
> * or TarMethodFile *, but I think it is clearly a mistake, and the
> approach I'm proposing here makes such mistakes more difficult to
> make.

+1.  I remember I thought the same thing when I was faced with the
code before.

> Aside from the stuff that I am complaining about here which is mostly
> stylistic, I think that the division of labor between receivelog.c and
> walmethods.c is questionable in a number of ways. There are things
> which are specific to one walmethod or the other that are handled in
> the common code (receivelog.c) rather than the type-specific code
> (walmethod.c), and in general it feels like receivelog.c knows way too
> much about what is really happening beneath the abstraction layer that
> walmethods.c supposedly creates. This comment is one of the clearer
> examples of this:
> 
>      /*
>       * When streaming to files, if an existing file exists we verify that it's
>       * either empty (just created), or a complete WalSegSz segment (in which
>       * case it has been created and padded). Anything else indicates a corrupt
>       * file. Compressed files have no need for padding, so just ignore this
>       * case.
>       *
>       * When streaming to tar, no file with this name will exist before, so we
>       * never have to verify a size.
>       */
> 
> There's nothing generic here. We're not describing an algorithm that
> could be used with any walmethod that might exist now or in the
> future. We're describing something that will produce the right result
> given the two walmethods we actually have and the actual behavior of
> the callbacks of each one. I don't really know what to do about this
> part of the problem; these pieces of code are deeply intertwined in
> complex ways that don't seem simple to untangle. Maybe I'll have a

I agree to the view. That part seems to be a part of
open_for_write()'s body functions. But, I'm not sure how we untangle
them at a glance, too.  In the first place, I'm not sure why we need
to do that despite the file going to be overwritten from the
beginning, though..

> better idea later, or perhaps someone else will. For now, I'd like to
> get some thoughts on the attached refactoring patches that deal with
> some more superficial aspects of the problem.

regares.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center