Horton

Thread

Commits

  1. walmethods.c/h: Make WalWriteMethod more object-oriented.

  2. walmethods.c/h: Make Walfile a struct, rather than a void *

  1. walmethods.c/h are doing some strange things

    Robert Haas <robertmhaas@gmail.com> — 2022-09-02T15:52:38Z

    Hi,

We have a number of places in the system where we are using
object-oriented design patterns. For example, a foreign data wrapper
returns a table of function pointers which are basically methods for
operating on a planner or executor node that corresponds to a foreign
table that uses that foreign data wrapper. More simply, a
TupleTableSlot or TableAmRoutine or bbstreamer or bbsink object
contains a pointer to a table of callbacks which are methods that can
be applied to that object. walmethods.c/h also try to do something
sort of like this, but I find the way that they do it really weird,
because while Create{Directory|Tar}WalMethod() does return a table of
callbacks, those callbacks aren't tied to any specific object.
Instead, each set of callbacks refers to the one and only object of
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.

I think it would be better to structure things so that Walfile and
WalWriteMethod function as abstract base classes; that is, each is a
struct containing those members that are common to all
implementations, and then each implementation extends that struct with
whatever additional members it needs. One advantage of this is that it
would allow us to simplify the communication between receivelog.c and
walmethods.c. Right now, for example, there's a get_current_pos()
method in WalWriteMethods. The way that works is that
WalDirectoryMethod has a struct where it stores a 'curpos' value that
is returned by this method, and WalTrMethod has a different struct
that also stores a 'currpos' value that is returned by this method.
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.

Attached are a couple of hastily-written patches implementing this.
There might be good arguments for more thoroughly renaming some of the
things these patches touch, but I thought that doing any more renaming
would make it less clear what the core of the change is, so I'm
posting it like this for now. One thing I noticed while writing these
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.

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

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  2. Re: walmethods.c/h are doing some strange things

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-12T15:24:32Z

    On 02.09.22 17:52, Robert Haas wrote:
> Attached are a couple of hastily-written patches implementing this.
> There might be good arguments for more thoroughly renaming some of the
> things these patches touch, but I thought that doing any more renaming
> would make it less clear what the core of the change is, so I'm
> posting it like this for now. One thing I noticed while writing these
> 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.

This direction does make sense IMO.

  3. Re: walmethods.c/h are doing some strange things

    velagandula sravan kumar <sravan_velag@yahoo.com> — 2022-09-15T08:52:33Z

    
From: Robert Haas <robertmhaas@gmail.com>
Date: Friday, 2 September 2022 at 9:23 PM
To: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: walmethods.c/h are doing some strange things
Hi,

We have a number of places in the system where we are using
object-oriented design patterns. For example, a foreign data wrapper
returns a table of function pointers which are basically methods for
operating on a planner or executor node that corresponds to a foreign
table that uses that foreign data wrapper. More simply, a
TupleTableSlot or TableAmRoutine or bbstreamer or bbsink object
contains a pointer to a table of callbacks which are methods that can
be applied to that object. walmethods.c/h also try to do something
sort of like this, but I find the way that they do it really weird,
because while Create{Directory|Tar}WalMethod() does return a table of
callbacks, those callbacks aren't tied to any specific object.
Instead, each set of callbacks refers to the one and only object of
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.

I think it would be better to structure things so that Walfile and
WalWriteMethod function as abstract base classes; that is, each is a
struct containing those members that are common to all
implementations, and then each implementation extends that struct with
whatever additional members it needs. One advantage of this is that it
would allow us to simplify the communication between receivelog.c and
walmethods.c. Right now, for example, there's a get_current_pos()
method in WalWriteMethods. The way that works is that
WalDirectoryMethod has a struct where it stores a 'curpos' value that
is returned by this method, and WalTrMethod has a different struct
that also stores a 'currpos' value that is returned by this method.
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.


Attached are a couple of hastily-written patches implementing this.
There might be good arguments for more thoroughly renaming some of the
things these patches touch, but I thought that doing any more renaming
would make it less clear what the core of the change is, so I'm
posting it like this for now. One thing I noticed while writing these
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 on being able to restrict making such mistakes. I had a quick look at the patch
and the refactoring makes sense.

Thanks,
Sravan Kumar
www.enterprisedb.com


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

Thanks,

--
Robert Haas
EDB: http://www.enterprisedb.com

  4. Re: walmethods.c/h are doing some strange things

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-16T01:39:29Z

    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

  5. Re: walmethods.c/h are doing some strange things

    Robert Haas <robertmhaas@gmail.com> — 2022-09-19T18:02:27Z

    On Thu, Sep 15, 2022 at 9:39 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> 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.

OK, I have committed the patches. Before doing that, I fixed a couple
of bugs in the first one, and did a little bit of rebasing of the
second one.

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

I suspect that we're going to have to untangle things bit by bit.

One place where I think we might be able to improve things is with the
handling of compression suffixes (e.g. .gz, .lz4) and temp suffixes
(e.g. .partial, .tmp). At present, responsibility for adding these
suffixes to pathnames is spread across receivelog.c and walmethods.c
in a way that, to me, looks pretty random. It's not exactly clear to
me what the best design is here right now, but I think either (A)
receivelog.c should take full responsibility for computing the exact
filename and walmethods.c should just blindly write the data into the
exact filename it's given, or else (B) receivelog.c should take no
responsibility for pathname construction and the fact that there is
pathname munging happening should be hidden inside walmethods.c. Right
now, walmethods.c is doing pathname munging, but the munging is
visible from receivelog.c, so the responsibility is spread across the
two files rather than being the sole responsibility of either.

What's also a little bit aggravating about this is that it doesn't
feel like we're accomplishing all that much code reuse here.
walmethods.c and receivelog.c are shared only between pg_basebackup
and pg_receivewal, but the tar method is used only by pg_basebackup.
The directory mode is shared, but actually the two programs need a
bunch of different things. pg_recievelog needs a bunch of logic to
deal with the possibility that it is receiving data into an existing
directory and that this directory might at any time start to be used
to feed a standby, or used for PITR. That's not an issue for
pg_basebackup: if it fails, the whole directory will be removed, so
there is no need to worry about fsyncs or padding or overwriting
existing files. On the other hand, pg_basebackup needs a bunch of
logic to create a .done file for each WAL segment which is not
required in the case of pg_receivewal. It feels like we've got as much
conditional logic as we do common logic...

-- 
Robert Haas
EDB: http://www.enterprisedb.com