On Thu, Sep 1, 2016 at 11:16 PM, Tom Lane wrote:
> Michael Paquier writes:
>> On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane wrote:
>>> Don't see the point really ... it's just more API churn without any
>>> very compelling reason.
>
>> OK, then no objection to your approach. At least I tried.
>
> OK
Michael Paquier writes:
> On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane wrote:
>> Don't see the point really ... it's just more API churn without any
>> very compelling reason.
> OK, then no objection to your approach. At least I tried.
OK, pushed my version then. I think we have now dealt with ev
On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane wrote:
> Michael Paquier writes:
>> Also, we could take one extra step forward then, and just introduce
>> ShmemAllocExtended that holds two flags as per the attached:
>> - SHMEM_ALLOC_ZERO that zeros all the fields
>> - SHMEM_ALLOC_NO_OOM that does not f
Michael Paquier writes:
> Also, we could take one extra step forward then, and just introduce
> ShmemAllocExtended that holds two flags as per the attached:
> - SHMEM_ALLOC_ZERO that zeros all the fields
> - SHMEM_ALLOC_NO_OOM that does not fail
Don't see the point really ... it's just more API c
On Thu, Sep 1, 2016 at 12:14 AM, Tom Lane wrote:
> I still think it'd be better to fix that as attached, because it
> represents a net reduction not net addition of code, and it provides
> a defense against future repetitions of the same omission. If only
> 4 out of 11 existing calls were properl
Michael Paquier writes:
> Which means that processes have an escape window when initializing
> shared memory by cleaning up the index if an entry cannot be found and
> then cannot be created properly. I don't think that it is a good idea
> to change that by forcing ShmemAlloc to fail. So I would t
On Wed, Aug 31, 2016 at 7:47 AM, Tom Lane wrote:
> Michael Paquier writes:
>> [ malloc-nulls-v5.patch ]
>
> I've committed some form of all of these changes
Thanks!
> except the one in
> adt/pg_locale.c. I'm not entirely sure whether we need to do anything
> about that at all, but if we do, th
On Wed, Aug 31, 2016 at 2:15 AM, Tom Lane wrote:
> Michael Paquier writes:
>> And with an actual patch things are better.
>
> Working through this patch, it suddenly strikes me that we are going
> about fixing the callers of simple_prompt the wrong way. The existing
> definition with returning a
Michael Paquier writes:
> [ malloc-nulls-v5.patch ]
I've committed some form of all of these changes except the one in
adt/pg_locale.c. I'm not entirely sure whether we need to do anything
about that at all, but if we do, this doesn't cut it:
thousands_sep = db_encoding_strdup(encoding, ex
Michael Paquier writes:
> And with an actual patch things are better.
Working through this patch, it suddenly strikes me that we are going
about fixing the callers of simple_prompt the wrong way. The existing
definition with returning a malloc'd string creates a hazard of malloc
failure, and it
Aleksander Alekseev writes:
> I suggest to keep ShmemAlloc as is for backward compatibility and
> introduce a new procedure ShmemAllocSafe.
I think that's about the worst of all possible worlds, as it guarantees
having to touch most call sites. If there were more than one known caller
that reall
Michael Paquier writes:
> On Tue, Aug 30, 2016 at 10:18 PM, Tom Lane wrote:
>> I think what we ought to do is make ShmemAlloc act like palloc
>> (ie throw error not return NULL), and remove the duplicated error
>> checks.
> The only reason why I did not propose that for ShmemAlloc is because
> o
> > I think what we ought to do is make ShmemAlloc act like palloc
> > (ie throw error not return NULL), and remove the duplicated error
> > checks. For the one caller that that would be bad for, we could
> > invent something like ShmemAllocNoError, or ShmemAllocExtended with
> > a no_error flag,
On Tue, Aug 30, 2016 at 10:18 PM, Tom Lane wrote:
> I think what we ought to do is make ShmemAlloc act like palloc
> (ie throw error not return NULL), and remove the duplicated error
> checks. For the one caller that that would be bad for, we could
> invent something like ShmemAllocNoError, or Sh
Michael Paquier writes:
>> I've just realized that there is also malloc-compatible ShmemAlloc().
>> Apparently it's return value sometimes is not properly checked too. See
>> attachment.
> The funny part here is that ProcGlobal->allProcs is actually handled,
> but not the two others. Well yes, yo
> >> And with an actual patch things are better.
> >
> > Currently I can't think of any further improvements. I even would dare
> > to say that patch is Ready for Committer.
>
> Thanks for the fruitful input by the way! You spotted many things.
Thank _you_ for paying attention for such issues in
On Tue, Aug 30, 2016 at 5:08 PM, Aleksander Alekseev
wrote:
>> > The funny part here is that ProcGlobal->allProcs is actually handled,
>> > but not the two others. Well yes, you are right, we really need to
>> > fail on FATAL for all of them if ShmemAlloc returns NULL as they
>> > involve the shme
> > The funny part here is that ProcGlobal->allProcs is actually handled,
> > but not the two others. Well yes, you are right, we really need to
> > fail on FATAL for all of them if ShmemAlloc returns NULL as they
> > involve the shmem initialization at postmaster startup.
>
> And with an actual p
On Tue, Aug 30, 2016 at 2:57 PM, Michael Paquier
wrote:
> The funny part here is that ProcGlobal->allProcs is actually handled,
> but not the two others. Well yes, you are right, we really need to
> fail on FATAL for all of them if ShmemAlloc returns NULL as they
> involve the shmem initialization
On Mon, Aug 29, 2016 at 11:26 PM, Tom Lane wrote:
> Aleksander Alekseev writes:
>>> if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>>> + {
>>> +free(prodesc);
>
>> I think that prodesc->user_proname and prodesc->internal_proname should
>> also be freed if they are not
On Mon, Aug 29, 2016 at 11:13 PM, Aleksander Alekseev
wrote:
>> Hello, Michael
>>
>> > I don't know how you did it, but this email has visibly broken the
>> > original thread. Did you change the topic name?
>>
>> I'm very sorry for this. I had no local copy of this thread. So I wrote a
>> new emai
Aleksander Alekseev writes:
>> if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>> + {
>> +free(prodesc);
> I think that prodesc->user_proname and prodesc->internal_proname should
> also be freed if they are not NULL's.
Hmm, this is kind of putting lipstick on a pig, i
> Hello, Michael
>
> > I don't know how you did it, but this email has visibly broken the
> > original thread. Did you change the topic name?
>
> I'm very sorry for this. I had no local copy of this thread. So I wrote a
> new email with the same subject hoping it will be OK. Apparently right
> In
Hello, Michael
> I don't know how you did it, but this email has visibly broken the
> original thread. Did you change the topic name?
I'm very sorry for this. I had no local copy of this thread. So I wrote a
new email with the same subject hoping it will be OK. Apparently right
In-Reply-To header
On Sat, Aug 27, 2016 at 10:24 PM, Michael Paquier
wrote:
> ./src/backend/postmaster/postmaster.c: userDoption =
> strdup(optarg);
> [...]
> ./src/backend/bootstrap/bootstrap.c:userDoption =
> strdup(optarg);
> [...]
> ./src/backend/tcop/postgres.c: use
On Sat, Aug 27, 2016 at 12:33 AM, Aleksander Alekseev
wrote:
> Your patch [1] was marked as "Needs review" so I decided to take a look.
Thanks for the input!
> It looks good to me. However I found dozens of places in PostgreSQL code
> that seem to have similar problem you are trying to fix [2].
Hello, Michael.
Your patch [1] was marked as "Needs review" so I decided to take a look.
It looks good to me. However I found dozens of places in PostgreSQL code
that seem to have similar problem you are trying to fix [2]. As far as I
understand these are only places left that don't check malloc/
On Thu, Aug 18, 2016 at 6:12 PM, Heikki Linnakangas wrote:
> On 06/22/2016 04:41 AM, Michael Paquier wrote:
>> make s
>> OK, there is not much that we can do here then. What about the rest?
>> Those seem like legit concerns to me.
>
>
> There's also a realloc() and an strdup() call in refint.c. Bu
On 06/22/2016 04:41 AM, Michael Paquier wrote:
On Tue, Jun 21, 2016 at 10:46 PM, Tom Lane wrote:
Michael Paquier writes:
- mcxt.c uses that, which is surprising:
@@ -704,7 +704,8 @@ MemoryContextCreate(NodeTag tag, Size size,
{
/* Special case for startup: use good ol' malloc */
On Wed, Jun 22, 2016 at 10:41 AM, Michael Paquier
wrote:
> OK, there is not much that we can do here then. What about the rest?
> Those seem like legit concerns to me.
Registered this one to the next CF as a bug fix:
https://commitfest.postgresql.org/10/653/
It would be better not to crash if we
On Tue, Jun 21, 2016 at 10:46 PM, Tom Lane wrote:
> Michael Paquier writes:
>> - mcxt.c uses that, which is surprising:
>> @@ -704,7 +704,8 @@ MemoryContextCreate(NodeTag tag, Size size,
>> {
>> /* Special case for startup: use good ol' malloc */
>> node = (MemoryContext) mall
Michael Paquier writes:
> - mcxt.c uses that, which is surprising:
> @@ -704,7 +704,8 @@ MemoryContextCreate(NodeTag tag, Size size,
> {
> /* Special case for startup: use good ol' malloc */
> node = (MemoryContext) malloc(needed);
> - Assert(node != NULL);
> + if (
Hi all,
While auditing the code, I got surprised that there are a couple of
code paths that do nothing for this error handling:
- pg_regress and isolationtester use malloc extensively, in case of
failure those would just crash crash. I think that it matters for
buildfarm members that are under mem
33 matches
Mail list logo
