You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@madlib.apache.org by iyerr3 <gi...@git.apache.org> on 2018/08/29 23:27:39 UTC
[GitHub] madlib pull request #316: Build: Disable AppendOnly if available
GitHub user iyerr3 opened a pull request:
https://github.com/apache/madlib/pull/316
Build: Disable AppendOnly if available
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/madlib/madlib feature/support_ao_storage
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/madlib/pull/316.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #316
----
commit 5d92943cc639609a0c498692c2416d7f7be201bc
Author: Rahul Iyer <ri...@...>
Date: 2018-08-29T23:23:04Z
Build: Disable AppendOnly if available
----
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/madlib/pull/316
Accidently created this PR. This should be ready to review in a couple of days.
---
[GitHub] madlib pull request #316: Build: Disable AppendOnly if available
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/316#discussion_r217260530
--- Diff: src/ports/postgres/modules/utilities/control.py_in ---
@@ -158,6 +159,61 @@ class MinWarning(ContextDecorator):
format(oldMsgLevel=self.oldMsgLevel))
+class AOControl(ContextDecorator):
+
+ """
+ @brief: A wrapper that enables/disables the AO storage option
+ """
+
+ def __init__(self, enable=False):
+ self.to_enable = enable
+ self.was_ao_enabled = False
+ self.guc_exists = True
+ self.storage_options_dict = dict()
+
+ def _parse_gp_default_storage_options(self, gp_default_storage_options_str):
+ """ Parse comma separated key=value pairs
+
+ Example:
+ appendonly=false,blocksize=32768,compresstype=none,checksum=true,orientation=row
+ """
+ self.storage_options_dict = extract_keyvalue_params(gp_default_storage_options_str)
+ self.storage_options_dict['appendonly'] = bool(
+ strtobool(self.storage_options_dict['appendonly']))
+
+ def _join_gp_defaut_storage_options(self):
+ return ','.join(['{0}={1}'.format(k, v)
+ for k, v in self.storage_options_dict.iteritems()])
+
+ def __enter__(self):
+ try:
+ _storage_options_str = plpy.execute(
+ "show gp_default_storage_options")[0]["gp_default_storage_options"]
+ self._parse_gp_default_storage_options(_storage_options_str)
+
+ # Set APPENDONLY=False after backing up existing value
+ self.was_ao_enabled = self.storage_options_dict['appendonly']
+ self.storage_options_dict['appendonly'] = self.to_enable
+ plpy.execute("set gp_default_storage_options={0}".
+ format(self._join_gp_defaut_storage_options()))
+ except plpy.SPIError:
+ self.guc_exists = False
+ finally:
+ return self
+
+ def __exit__(self, *args):
+ if args and args[0]:
+ # an exception was raised in code. We return False so that any
+ # exception is re-raised after exit. The transaction will not
+ # commit leading to reset of parameter value.
--- End diff --
The GUC value should also reset if transaction does not commit. Ideally, this context manager should be called at the highest level. However, as you mention, it's possible it's not used correctly, with the raised exception caught and ignored. I have updated the code to perform the reset before re-raising the exception.
---
[GitHub] madlib pull request #316: Build: Disable AppendOnly if available
Posted by reductionista <gi...@git.apache.org>.
Github user reductionista commented on a diff in the pull request:
https://github.com/apache/madlib/pull/316#discussion_r217537741
--- Diff: src/ports/postgres/modules/utilities/control.py_in ---
@@ -158,6 +159,61 @@ class MinWarning(ContextDecorator):
format(oldMsgLevel=self.oldMsgLevel))
+class AOControl(ContextDecorator):
+
+ """
+ @brief: A wrapper that enables/disables the AO storage option
+ """
+
+ def __init__(self, enable=False):
+ self.to_enable = enable
+ self.was_ao_enabled = False
+ self.guc_exists = True
+ self.storage_options_dict = dict()
+
+ def _parse_gp_default_storage_options(self, gp_default_storage_options_str):
+ """ Parse comma separated key=value pairs
+
+ Example:
+ appendonly=false,blocksize=32768,compresstype=none,checksum=true,orientation=row
+ """
+ self.storage_options_dict = extract_keyvalue_params(gp_default_storage_options_str)
+ self.storage_options_dict['appendonly'] = bool(
+ strtobool(self.storage_options_dict['appendonly']))
+
+ def _join_gp_defaut_storage_options(self):
+ return ','.join(['{0}={1}'.format(k, v)
+ for k, v in self.storage_options_dict.iteritems()])
+
+ def __enter__(self):
+ try:
+ _storage_options_str = plpy.execute(
+ "show gp_default_storage_options")[0]["gp_default_storage_options"]
+ self._parse_gp_default_storage_options(_storage_options_str)
+
+ # Set APPENDONLY=False after backing up existing value
--- End diff --
Rest looks good, I will approve as soon as you update this comment.
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/316
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/666/
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/316
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/675/
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/316
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/663/
---
[GitHub] madlib pull request #316: Build: Disable AppendOnly if available
Posted by reductionista <gi...@git.apache.org>.
Github user reductionista commented on a diff in the pull request:
https://github.com/apache/madlib/pull/316#discussion_r215104112
--- Diff: src/ports/postgres/modules/utilities/control.py_in ---
@@ -158,6 +159,61 @@ class MinWarning(ContextDecorator):
format(oldMsgLevel=self.oldMsgLevel))
+class AOControl(ContextDecorator):
+
+ """
+ @brief: A wrapper that enables/disables the AO storage option
+ """
+
+ def __init__(self, enable=False):
+ self.to_enable = enable
+ self.was_ao_enabled = False
+ self.guc_exists = True
+ self.storage_options_dict = dict()
+
+ def _parse_gp_default_storage_options(self, gp_default_storage_options_str):
+ """ Parse comma separated key=value pairs
+
+ Example:
+ appendonly=false,blocksize=32768,compresstype=none,checksum=true,orientation=row
+ """
+ self.storage_options_dict = extract_keyvalue_params(gp_default_storage_options_str)
+ self.storage_options_dict['appendonly'] = bool(
+ strtobool(self.storage_options_dict['appendonly']))
+
+ def _join_gp_defaut_storage_options(self):
+ return ','.join(['{0}={1}'.format(k, v)
+ for k, v in self.storage_options_dict.iteritems()])
+
+ def __enter__(self):
+ try:
+ _storage_options_str = plpy.execute(
+ "show gp_default_storage_options")[0]["gp_default_storage_options"]
+ self._parse_gp_default_storage_options(_storage_options_str)
+
+ # Set APPENDONLY=False after backing up existing value
+ self.was_ao_enabled = self.storage_options_dict['appendonly']
+ self.storage_options_dict['appendonly'] = self.to_enable
+ plpy.execute("set gp_default_storage_options={0}".
+ format(self._join_gp_defaut_storage_options()))
+ except plpy.SPIError:
+ self.guc_exists = False
+ finally:
+ return self
+
+ def __exit__(self, *args):
+ if args and args[0]:
+ # an exception was raised in code. We return False so that any
+ # exception is re-raised after exit. The transaction will not
+ # commit leading to reset of parameter value.
--- End diff --
Is it guaranteed that a raised exception will result in a reset of appendonly back to its original value? Seems like the "set gp_default_storage_options=" command executed in the __enter__ block may not get reverted when an exception is raised, even if the transaction that follows is reverted.
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/316
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/661/
---
[GitHub] madlib pull request #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/madlib/pull/316
---
[GitHub] madlib pull request #316: Build: Disable AppendOnly if available
Posted by reductionista <gi...@git.apache.org>.
Github user reductionista commented on a diff in the pull request:
https://github.com/apache/madlib/pull/316#discussion_r215102984
--- Diff: src/ports/postgres/modules/utilities/control.py_in ---
@@ -158,6 +159,61 @@ class MinWarning(ContextDecorator):
format(oldMsgLevel=self.oldMsgLevel))
+class AOControl(ContextDecorator):
+
+ """
+ @brief: A wrapper that enables/disables the AO storage option
+ """
+
+ def __init__(self, enable=False):
+ self.to_enable = enable
+ self.was_ao_enabled = False
+ self.guc_exists = True
+ self.storage_options_dict = dict()
+
+ def _parse_gp_default_storage_options(self, gp_default_storage_options_str):
+ """ Parse comma separated key=value pairs
+
+ Example:
+ appendonly=false,blocksize=32768,compresstype=none,checksum=true,orientation=row
+ """
+ self.storage_options_dict = extract_keyvalue_params(gp_default_storage_options_str)
+ self.storage_options_dict['appendonly'] = bool(
+ strtobool(self.storage_options_dict['appendonly']))
+
+ def _join_gp_defaut_storage_options(self):
+ return ','.join(['{0}={1}'.format(k, v)
+ for k, v in self.storage_options_dict.iteritems()])
+
+ def __enter__(self):
+ try:
+ _storage_options_str = plpy.execute(
+ "show gp_default_storage_options")[0]["gp_default_storage_options"]
+ self._parse_gp_default_storage_options(_storage_options_str)
+
+ # Set APPENDONLY=False after backing up existing value
--- End diff --
What about AOControl[True]? Then it will set APPENDONLY=True after backing up, right?
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/316
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/665/
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/316
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/676/
---
[GitHub] madlib pull request #316: Build: Disable AppendOnly if available
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/316#discussion_r217260262
--- Diff: src/ports/postgres/modules/utilities/control.py_in ---
@@ -158,6 +159,61 @@ class MinWarning(ContextDecorator):
format(oldMsgLevel=self.oldMsgLevel))
+class AOControl(ContextDecorator):
+
+ """
+ @brief: A wrapper that enables/disables the AO storage option
+ """
+
+ def __init__(self, enable=False):
+ self.to_enable = enable
+ self.was_ao_enabled = False
+ self.guc_exists = True
+ self.storage_options_dict = dict()
+
+ def _parse_gp_default_storage_options(self, gp_default_storage_options_str):
+ """ Parse comma separated key=value pairs
+
+ Example:
+ appendonly=false,blocksize=32768,compresstype=none,checksum=true,orientation=row
+ """
+ self.storage_options_dict = extract_keyvalue_params(gp_default_storage_options_str)
+ self.storage_options_dict['appendonly'] = bool(
+ strtobool(self.storage_options_dict['appendonly']))
+
+ def _join_gp_defaut_storage_options(self):
+ return ','.join(['{0}={1}'.format(k, v)
+ for k, v in self.storage_options_dict.iteritems()])
+
+ def __enter__(self):
+ try:
+ _storage_options_str = plpy.execute(
+ "show gp_default_storage_options")[0]["gp_default_storage_options"]
+ self._parse_gp_default_storage_options(_storage_options_str)
+
+ # Set APPENDONLY=False after backing up existing value
--- End diff --
Yes, the comment is incorrect. I will change it to "Set APPENDONLY to input value"
---
[GitHub] madlib issue #316: Build: Disable AppendOnly if available
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/316
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/674/
---