Re: Improve the error message for logical replication of regular column to generated column.

On Fri, Nov 15, 2024 at 2:07 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Nov 15, 2024 at 6:10 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > 3. A different approach?
> >
> > TBH, is introducing a whole new error message even a good idea?
> >
> > Now there are going to be two separate error messages where previously
> > there was only one. So if the table has multiple problems at the same
> > time then still only one of them can "win". i.e. you have to either
> > report the "generated columns" problem 1st or the "missing columns"
> > problem 1st -- either way that might not be a good user experience
> > because they might be unaware of multiple problems until they try the
> > CREATE SUBSCRIPTION a 2nd time and then it fails a 2nd time with the
> > other kind of error! That could be annoying.
> >
>
> I don't know why the user needs to perform CREATE SUBSCRIPTION
> multiple times to see this. IIUC, this error will happen in the apply
> worker and after fixing the first, the user should see the second. I
> think this can happen in other ways in apply worker as well.

Yeah, I was thinking more of the scenario where the CREATE
SUBSCRIPTION gave the immediate error, so the user panics and does
DROP SUBSCRIPTION to give them all the time they need while they fix
the problem. Then they won't see the second problem until they
recreate the subscription.

But if they just are happy to leave the original CREATE SUBSCRIPTION
failing continuously while they fix the first problem then I think you
are correct --- the error should just fall through further to show the
next problem.

>
> > A better solution may be just to *combine* everything, so the user
> > only has to deal with one error. IIUC that's what is already happening
> > in master code, so this patch doesn't need to do anything except make
> > a quite trivial change to the wording of the existing error message.
> >
> > For example:
> > BEFORE
> > errmsg_plural("logical replication target relation \"%s.%s\" is
> > missing replicated column: %s",
> > "logical replication target relation \"%s.%s\" is
> > missing replicated columns: %s",
> > SUGGESTION
> > errmsg_plural("logical replication target relation \"%s.%s\" has
> > missing or generated replicated column: %s",
> > "logical replication target relation \"%s.%s\" has
> > missing or generated replicated columns: %s",
> >
>
> With this, we can combine two different ERRORs into one but it won't
> be evident if the column name referred in the message is generated or
> missing. I see your point but combining two different errors into one
> is also confusing. We can try to add more checks to make this
> distinction clear but it doesn't seem worth the effort and complexity.
> Also, it is not clear whether combining different ERRORs is a good
> idea in the first place.
>

I don't know if it needs to be spelled out explicitly in the message
which is which because the user will surely know their own subscriber
table definition, so it will be quite obvious to them if a named
column is missing or generated.

======
Kind Regards,
Peter Smith.
Fujitsu Australia