Date: Mon, 15 Nov 2021 17:26:41 +0100
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>,
	Mark Dilger <mark.dilger@enterprisedb.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	tushar <tushar.ahuja@enterprisedb.com>
Subject: Re: refactoring basebackup.c
Message-ID: <20211115162641.dmo6l32fklh64gnw@localhost>
References: 
 <CAOgcT0OarU7csuf6Obgmg96DdjpGKUSETx5d6ss3eJx7VD+bdA@mail.gmail.com>
 <CA+TgmoZEzVs5s-Y-mCMb=pABTdaney=EgipH575z4b3Q=+oW+A@mail.gmail.com>
 <CAOgcT0N1zxV0swyR3hDVVXjvun-zO1HhDx4+_F4nUiMv4RiUNw@mail.gmail.com>
 <CA+Tgmob6K9u+YOtpK56L1q6q4JnO6B0_N577X60TzVmi8fyC3A@mail.gmail.com>
 <CA+TgmobnZT4JKVNfAhWru951HA8P7SBNdCbJvCxAXu0=2+6WdA@mail.gmail.com>
 <CAOgcT0P0OGFfNhCKRUyV-rGMXk+JnsYUKGF3tPYhog2c1EQAmg@mail.gmail.com>
 <CA+TgmoY_kqYZdpuXLvcX9wj=8Lq6votwT-UJyMDE0btkOTyiWQ@mail.gmail.com>
 <CAOgcT0OeWuX+O=-5OTL8VWdjCaCh0HKDoKHDRq1zUfUrdXCnsw@mail.gmail.com>
 <CA+TgmoZvqk7UuzxsX1xjJRmMGkqoUGYTZLDCH8SmU1xTPr1Xig@mail.gmail.com>
 <CA+TgmobrOXbDh+hCzzVkD3weV3R-QRy3SPa=FRb_Rv9wF5iPJw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmobrOXbDh+hCzzVkD3weV3R-QRy3SPa=FRb_Rv9wF5iPJw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20211115162641.dmo6l32fklh64gnw%40localhost>
Precedence: bulk

> On Fri, Nov 05, 2021 at 11:50:01AM -0400, Robert Haas wrote:
> On Tue, Nov 2, 2021 at 10:32 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > Meanwhile, I think it's probably OK for me to go ahead and commit
> > 0001-0003 from my patches at this point, since it seems we have pretty
> > good evidence that the abstraction basically works, and there doesn't
> > seem to be any value in holding off and maybe having to do a bunch
> > more rebasing.
>
> I went ahead and committed 0001 and 0002, but got nervous about
> proceeding with 0003.

Hi,

I'm observing a strange issue which I can only relate to bef47ff85d
where bbsink abstraction was introduced. The problem is about failing
assertion when doing:

    DETAIL:  Failed process was running: BASE_BACKUP ( LABEL 'pg_basebackup base backup',  PROGRESS,  WAIT 0,  MAX_RATE 102400,  MANIFEST 'yes')

Walsender tries to send a backup manifest, but crashes on the trottling sink:

    #2  0x0000560857b551af in ExceptionalCondition (conditionName=0x560857d15d27 "sink->bbs_next != NULL", errorType=0x560857d15c23 "FailedAssertion", fileName=0x560857d15d15 "basebackup_sink.c", lineNumber=91) at assert.c:69
    #3  0x0000560857918a94 in bbsink_forward_manifest_contents (sink=0x5608593f73f8, len=32768) at basebackup_sink.c:91
    #4  0x0000560857918d68 in bbsink_throttle_manifest_contents (sink=0x5608593f7450, len=32768) at basebackup_throttle.c:125
    #5  0x00005608579186d0 in bbsink_manifest_contents (sink=0x5608593f7450, len=32768) at ../../../src/include/replication/basebackup_sink.h:240
    #6  0x0000560857918b1b in bbsink_forward_manifest_contents (sink=0x5608593f74e8, len=32768) at basebackup_sink.c:94
    #7  0x0000560857911edc in bbsink_manifest_contents (sink=0x5608593f74e8, len=32768) at ../../../src/include/replication/basebackup_sink.h:240
    #8  0x00005608579129f6 in SendBackupManifest (manifest=0x7ffdaea9d120, sink=0x5608593f74e8) at backup_manifest.c:373

Looking at the similar bbsink_throttle_archive_contents it's not clear
why comments for both functions (archive and manifest throttling) say
"pass archive contents to next sink", but only bbsink_throttle_manifest_contents
does pass bbs_next into the bbsink_forward_manifest_contents. Is it
supposed to be like that? Passing the same sink object instead the next
one into bbsink_forward_manifest_contents seems to solve the problem in
this case.