Обсуждение: [PATCH] Small refactoring of inval.c and inval.h

Hi,

The proposed patch is a small refactoring of inval.{c,h}:

"""
    The "public functions" separator comment doesn't reflect reality anymore.
    We could rearrange the order of the functions. However, this would
complicate
    back-porting of the patches, thus removing the comment instead.

    InvalidateSystemCachesExtended() is an internal function of inval.c. Make it
    static.
"""

-- 
Best regards,
Aleksander Alekseev

On 2023-Jul-25, Aleksander Alekseev wrote:

> Hi,
> 
> The proposed patch is a small refactoring of inval.{c,h}:
> 
> """
>     The "public functions" separator comment doesn't reflect reality anymore.
>     We could rearrange the order of the functions. However, this would
> complicate
>     back-porting of the patches, thus removing the comment instead.
> 
>     InvalidateSystemCachesExtended() is an internal function of inval.c. Make it
>     static.
> """

Hmm, this *Extended function looks a bit funny, and I think it's because
it's part of a backpatched bugfix that didn't want to modify ABI.  If
we're modifying this code, maybe we should get rid of the shim, that is,
move the boolean argument to InvalidateSystemCaches() and get rid of the
*Extended() version altogether.

As for the /*--- public functions ---*/ comment, that one was just not
moved by b89e151054a0, which should have done so; but even at that
point, it had already been somewhat broken by the addition of
PrepareInvalidationState() (a static function) in 6cb4afff33ba below it.
If we really want to clean this up, we could move PrepareInvalidationState()
to just below RegisterSnapshotInvalidation() and move the "public
functions" header to just below it (so that it appears just before
InvalidateSystemCaches).

If we just remove the "public functions" comment, then we're still
inside a section that starts with the "private support functions"
comment, which seems even worse to me than the current situation.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

Hi Alvaro,

Thanks for your feedback.

> Hmm, this *Extended function looks a bit funny, and I think it's because
> it's part of a backpatched bugfix that didn't want to modify ABI.  If
> we're modifying this code, maybe we should get rid of the shim, that is,
> move the boolean argument to InvalidateSystemCaches() and get rid of the
> *Extended() version altogether.

This would affect quite a few places where InvalidateSystemCaches() is
called, and we will end-up with some sort of wrapper anyway. The
reason is that InvalidateSystemCaches() is used as a callback passed
to ReceiveSharedInvalidMessages():

```
    ReceiveSharedInvalidMessages(LocalExecuteInvalidationMessage,
                                 InvalidateSystemCaches);
```

Unless of course we want to change its signature too. I don't think
this is going to be a good API change.

> As for the /*--- public functions ---*/ comment, that one was just not
> moved by b89e151054a0, which should have done so; but even at that
> point, it had already been somewhat broken by the addition of
> PrepareInvalidationState() (a static function) in 6cb4afff33ba below it.
> If we really want to clean this up, we could move PrepareInvalidationState()
> to just below RegisterSnapshotInvalidation() and move the "public
> functions" header to just below it (so that it appears just before
> InvalidateSystemCaches).
>
> If we just remove the "public functions" comment, then we're still
> inside a section that starts with the "private support functions"
> comment, which seems even worse to me than the current situation.

Fixed.

-- 
Best regards,
Aleksander Alekseev

On Tue, Jul 25, 2023 at 06:38:46PM +0300, Aleksander Alekseev wrote:
> Unless of course we want to change its signature too. I don't think
> this is going to be a good API change.

 extern void InvalidateSystemCaches(void);
-extern void InvalidateSystemCachesExtended(bool debug_discard);

Indeed, that looks a bit strange, but is there a strong need in
removing it, as you are proposing?  There is always a risk that this
could be called by some out-of-core code.  This is one of these
things where we could break something just for the sake of breaking
it, so there is no real benefit IMO.

>> As for the /*--- public functions ---*/ comment, that one was just not
>> moved by b89e151054a0, which should have done so; but even at that
>> point, it had already been somewhat broken by the addition of
>> PrepareInvalidationState() (a static function) in 6cb4afff33ba below it.
>> If we really want to clean this up, we could move PrepareInvalidationState()
>> to just below RegisterSnapshotInvalidation() and move the "public
>> functions" header to just below it (so that it appears just before
>> InvalidateSystemCaches).
>>
>> If we just remove the "public functions" comment, then we're still
>> inside a section that starts with the "private support functions"
>> comment, which seems even worse to me than the current situation.
>
> Fixed.

This part looks OK to me, so I'll see about applying this part of the
patch.
--
Michael

Hi,

>  extern void InvalidateSystemCaches(void);
> -extern void InvalidateSystemCachesExtended(bool debug_discard);
>
> Indeed, that looks a bit strange, but is there a strong need in
> removing it, as you are proposing?  There is always a risk that this
> could be called by some out-of-core code.  This is one of these
> things where we could break something just for the sake of breaking
> it, so there is no real benefit IMO.

Fair enough, here is the corrected patch.

-- 
Best regards,
Aleksander Alekseev

On Mon, Nov 06, 2023 at 01:17:12PM +0300, Aleksander Alekseev wrote:
> Fair enough, here is the corrected patch.

Okay for me, so applied.  Thanks!
--
Michael