Hi, On Thu, Sep 25, 2025 at 10:16:35AM +0530, Ashutosh Bapat wrote: > On Wed, Sep 24, 2025 at 8:11 PM Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > > > === 4 > > > > +extern Size ReorderBufferChangeSize(ReorderBufferChange *change); > > > > Another approach could be to pass the change's size as an argument to the > > callbacks? That would avoid to expose ReorderBufferChangeSize publicly. > > Do you see any problem in exposing ReorderBufferChangeSize(). It's a > pretty small function and may be quite handy to output plugins > otherwise as well. And we expose many ReorderBuffer related functions; > so this isn't the first.
Right. I don't see a problem per say, just thinking that the less we expose publicly to be used by extensions/plugins, the better. > If we were to do as you say, it will change other external facing APIs > like change_cb(). Output plugins will need to change their code > accordingly even when they don't want to support plugin statistics. Correct. > Given that we have made maintaining plugin statistics optional, > forcing API change does not make sense. For example, test_decoding > which does not filter anything would unnecessarily have to change its > code. That's right. > I considered adding a field size to ReorderBufferChange itself. But > that means we increase the amount of memory used in the reorder > buffer, which seems to have become prime estate these days. So > rejected that idea as well. > > Advantage of this change is that the minimal cost of calculating the > size and maintaining the code change is incurred only when filtering > happens, by the plugins which want to filter and maintain statistics. Yes, anyway as it's unlikely that we have to fix a bug in a minor release that would need a signature change to ReorderBufferChangeSize(), I think that's fine as proposed. > > > > === 5 > > > > ctx->output_plugin_private = data; > > + ctx->stats = palloc0(sizeof(OutputPluginStats)); > > > > I was wondering if we need to free this in pg_decode_shutdown, but it looks > > like it's done through FreeDecodingContext() anyway. > > That's correct. Even output_plugin_private is freed when the decoding > memory context is freed. > > Thanks for the review comments. I have addressed the comments in my > repository and the changes will be included in the next set of > patches. Thanks! > Do you have any further review comments? Not right now. I'll give it another look by early next week the latest. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
