On Thu, Dec 21, 2023 at 7:16 PM Emre Hasegeli wrote:
>
> Fixed versions are attached.
>
Pushed!
--
With Regards,
Amit Kapila.
> But the xref seems present only in the master/v16/v15 patches, but not
> for the earlier patches v14/v13/v12. Why not?
I missed it.
> But the change was only in the patches v14 onwards. Although the new
> error message was only added for HEAD, isn't it still correct to say
> "A valid version
On Thu, Dec 21, 2023 at 2:58 AM Emre Hasegeli wrote:
>
> > We don't expect unrecognized option here and for such a thing, we use
> > elog in the code. See the similar usage in
> > parseCreateReplSlotOptions().
>
> "pgoutput" is useful for a lot of applications other than our logical
> replication
> We don't expect unrecognized option here and for such a thing, we use
> elog in the code. See the similar usage in
> parseCreateReplSlotOptions().
"pgoutput" is useful for a lot of applications other than our logical
replication subscriber. I think we should expect anything and handle
errors
On Tue, Dec 19, 2023 at 12:55 PM Amit Kapila wrote:
>
> I think we should move to 0002 patch now. In that, I suggest preparing
> separate back branch patches.
>
Emre, are you planning to share back-branch patches?
--
With Regards,
Amit Kapila.
On Tue, Dec 19, 2023 at 6:25 PM Amit Kapila wrote:
>
> On Tue, Dec 19, 2023 at 12:07 PM Peter Smith wrote:
> >
> > On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli wrote:
> > >
> > > > Fair enough. I think we should push your first patch only in HEAD as
> > > > this is a minor improvement over the
On Tue, Dec 19, 2023 at 12:07 PM Peter Smith wrote:
>
> On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli wrote:
> >
> > > Fair enough. I think we should push your first patch only in HEAD as
> > > this is a minor improvement over the current behaviour. What do you
> > > think?
> >
> > I agree.
>
>
On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli wrote:
>
> > Fair enough. I think we should push your first patch only in HEAD as
> > this is a minor improvement over the current behaviour. What do you
> > think?
>
> I agree.
Patch 0001
AFAICT parse_output_parameters possible errors are never
> Fair enough. I think we should push your first patch only in HEAD as
> this is a minor improvement over the current behaviour. What do you
> think?
I agree.
On Mon, Dec 18, 2023 at 1:08 PM Emre Hasegeli wrote:
>
> > I found the existing error code appropriate because for syntax
> > specification, either we need to mandate this at the grammar level or
> > at the API level. Also, I think we should give a message similar to an
> > existing message:
> This change (Required in between two sentences) looks slightly odd to
> me. Can we instead extend the second line to something like: "This
> parameter is required, and the individual publication names are ...".
> Similarly we can adjust the proto_vesion explanation.
I don't think it's an
> I found the existing error code appropriate because for syntax
> specification, either we need to mandate this at the grammar level or
> at the API level. Also, I think we should give a message similar to an
> existing message: "publication_names parameter missing". For example,
> we can say,
On Fri, Dec 15, 2023 at 7:06 PM Emre Hasegeli wrote:
>
>
> > SUGGESTION
> > -proto_version
> > -publication_names
> > -binary
> > -messages
> > -origin
> >
> > Requires minimum protocol version 2:
> > -streaming (boolean)
> >
> > Requires minimum protocol version 3:
> > -two_phase
> >
> >
On Fri, Dec 15, 2023 at 7:06 PM Emre Hasegeli wrote:
>
> > I saw that the original "publication_names" error was using
> > errcode(ERRCODE_INVALID_PARAMETER_VALUE), but TBH since there is no
> > option given at all I felt ERRCODE_SYNTAX_ERROR might be more
> > appropriate errcode for those 2
On Mon, Dec 18, 2023 at 11:35 AM Peter Smith wrote:
>
> Hi, I had a look at the latest v02 patches.
>
> On Sat, Dec 16, 2023 at 12:36 AM Emre Hasegeli wrote:
> >
> > > OK, to deal with that can't you just include "origin" in the first
> > > group which has no special protocol requirements?
> >
>
Hi, I had a look at the latest v02 patches.
On Sat, Dec 16, 2023 at 12:36 AM Emre Hasegeli wrote:
>
> > OK, to deal with that can't you just include "origin" in the first
> > group which has no special protocol requirements?
>
> I think it'd be confusing because the option is not available
> I saw that the original "publication_names" error was using
> errcode(ERRCODE_INVALID_PARAMETER_VALUE), but TBH since there is no
> option given at all I felt ERRCODE_SYNTAX_ERROR might be more
> appropriate errcode for those 2 mandatory option errors.
It makes sense to me. Changed.
> 2.
>
>
Thanks for the update. Here are some more review comments for the v01* patches.
//
Patch v00-0001
v01 modified the messages more than I was expecting, although what you
did looks fine to me.
~~~
1.
+ /* Check required options */
+ if (!protocol_version_given)
+ ereport(ERROR,
+
> I agree that we missed updating the parameters of the Logical
> Streaming Replication Protocol documentation. I haven't reviewed all
> the details yet but one minor thing that caught my attention while
> looking at your patch is that we can update the exact additional
> information we started to
> To reduce translation efforts, perhaps it is better to arrange for
> these to share a common message.
Good idea. I've done so.
> Also, I am unsure whether to call these "parameters" or "options" -- I
> wanted to call them parameters like in the documentation, but every
> other message in this
Hi, here are some initial review comments.
//
Patch v00-0001
1.
+
+ /* Check required parameters */
+ if (!protocol_version_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("proto_version parameter missing")));
+ if (!publication_names_given)
+ ereport(ERROR,
+
On Wed, Dec 13, 2023 at 9:25 PM Emre Hasegeli wrote:
>
> I noticed that Logical Streaming Replication Protocol documentation
> [1] is missing the options added to "pgoutput" since version 14. A
> patch is attached to fix it together with another small one to give a
> nice error when
22 matches
Mail list logo
