Hello, On Sat, Oct 19, 2019, 3:27 PM Tomas Vondra <tomas.von...@2ndquadrant.com> wrote:
> On Sat, Oct 19, 2019 at 12:47:39PM -0400, Andrew Dunstan wrote: > > > >On 10/19/19 12:32 PM, David G. Johnston wrote: > >> On Sat, Oct 19, 2019 at 9:19 AM Tomas Vondra > >> <tomas.von...@2ndquadrant.com <mailto:tomas.von...@2ndquadrant.com>> > >> wrote: > >> > >> > > >> >We invented jsonb_set() (credit to Dmitry Dolgov). And we've had it > >> >since 9.5. That's five releases ago. So it's a bit late to be > >> coming to > >> >us telling us it's not safe (according to your preconceptions of > >> what it > >> >should be doing). > >> > > >> > >> > >> There have been numerous complaints and questions about this behavior > >> in those five years; and none of the responses to those defenses has > >> actually made the current behavior sound beneficial but rather have > >> simply said "this is how it works, deal with it". > > > > > >I haven't seen a patch, which for most possible solutions should be > >fairly simple to code. This is open source. Code speaks louder than > >complaints. > > > > IMHO that might be a bit too harsh - I'm not surprised no one sent a > patch when we're repeatedly telling people "you're holding it wrong". > Without a clear consensus what the "correct" behavior is, I wouldn't > send a patch either. > > > > >> > >> > > >> >We could change it prospectively (i.e. from release 13 on) if we > >> choose. > >> >But absent an actual bug (i.e. acting contrary to documented > >> behaviour) > >> >we do not normally backpatch such changes, especially when there > is a > >> >simple workaround for the perceived problem. And it's that policy > >> that > >> >is in large measure responsible for Postgres' deserved reputation > for > >> >stability. > >> > > >> > >> Yeah. > >> > >> > >> Agreed, this is v13 material if enough people come on board to support > >> making a change. > > > > > > > >We have changed such things in the past. But maybe a new function might > >be a better way to go. I haven't given it enough thought yet. > > > > I think the #1 thing we should certainly do is explaining the behavior > in the docs. > > > > > > >> > >> >And if we were to change it I'm not at all sure that we should do > >> it the > >> >way that's suggested here, which strikes me as no more intuitive > than > >> >the current behaviour. Rather I think we should possibly fill in > >> a json > >> >null in the indicated place. > >> > > >> > >> Not sure, but that seems rather confusing to me, because it's > >> mixing SQL > >> NULL and JSON null, i.e. it's not clear to me why > >> > >> [...] > >> > >> But I admit it's quite subjective. > >> > >> > >> Providing SQL NULL to this function and asking it to do something with > >> that is indeed subjective - with no obvious reasonable default, and I > >> agree that "return a NULL" while possible consistent is probably the > >> least useful behavior that could have been chosen. We should never > >> have allowed an SQL NULL to be an acceptable argument in the first > >> place, and can reasonably safely and effectively prevent it going > >> forward. Then people will have to explicitly code what they want to > >> do if their data and queries present this invalid unknown data to the > >> function. > >> > >> > > > >How exactly do we prevent a NULL being passed as an argument? The only > >thing we could do would be to raise an exception, I think. That seems > >like a fairly ugly thing to do, I'd need a h3eck of a lot of convincing. > > > > I don't know, but if we don't know what the "right" behavior with NULL > is, is raising an exception really that ugly? > Raising an exception at least would prevent people from blanking their column out unintentionally. And I am willing to write a patch to do that if we have consensus on how to change it. Ariadne
