zabetak commented on code in PR #6825: URL: https://github.com/apache/hive/pull/6825#discussion_r4154386091
########## standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/postgresql.conf: ########## @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# general + +listen_addresses = '*' +dynamic_shared_memory_type = posix + +# performance + +work_mem = 256MB +shared_buffers = 128MB + +# reduce WAL and archiving + +checkpoint_timeout=1d +max_wal_size = 64MB +min_wal_size = 32MB +wal_level = minimal +max_wal_senders = 0 +wal_buffers = -1 +archive_mode = off +archive_command = '/bin/true' Review Comment: I don't think this is very likely to happen given that it would still be debatable on where the archives should go by default. Moreover, based on [Postgres doc](https://www.postgresql.org/docs/18/runtime-config-wal.html#GUC-ARCHIVE-MODE) the archive mode cannot be enabled if `wal_level = minimal` defined above. ########## standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/entrypoint.sh: ########## @@ -13,10 +15,9 @@ # See the License for the specific language governing permissions and # limitations under the License. -#!/bin/bash +if [ -f /tmp/metastore_db.zstd ]; then + zstdcat /tmp/metastore_db.zstd | tar -C /var/lib/postgresql/ -x + rm /tmp/metastore_db.zstd +fi Review Comment: OK, I understood. Basically by doing the decompression on the entrypoint and not as part of the Dockerfile you can reduce the size of the image by paying a small decompression overhead on startup. Nice trick! ########## standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/Dockerfile: ########## @@ -14,9 +14,16 @@ # See the License for the specific language governing permissions and # limitations under the License. # -FROM postgres:12.3 +FROM postgres:18-alpine + +ADD https://github.com/thomasrebele/hive-postgres-metastore/releases/download/tpcds-30tb-histogram-1.0/metastore_tpcds30tb_with_histograms.raw_db.zstd /tmp/metastore_db.zstd Review Comment: ## Option 1 If you combine the download step with chown you can reduce the image size by ~500MB: ``` RUN wget -O /tmp/metastore_db.zstd https://github.com/thomasrebele/hive-postgres-metastore/releases/download/tpcds-30tb-histogram-1.0/metastore_tpcds30tb_with_histograms.raw_db.zstd \ && echo '0423efc0cbc554611ac3bbf5e7919b050767bf96 /tmp/metastore_db.zstd' > /tmp/metastore_db.sha1 \ && sha1sum -c /tmp/metastore_db.sha1 \ && chown postgres:postgres /tmp/metastore_db.zstd ``` ## Option 2 You could go a bit further and do the decompression together with the download and get the same size that you have now without a need for custom entrypoint. ``` RUN wget -O /tmp/metastore_db.zstd https://github.com/thomasrebele/hive-postgres-metastore/releases/download/tpcds-30tb-histogram-1.0/metastore_tpcds30tb_with_histograms.raw_db.zstd \ && echo '0423efc0cbc554611ac3bbf5e7919b050767bf96 /tmp/metastore_db.zstd' > /tmp/metastore_db.sha1 \ && sha1sum -c /tmp/metastore_db.sha1 \ && chown postgres:postgres /tmp/metastore_db.zstd \ && zstdcat /tmp/metastore_db.zstd | tar -C /var/lib/postgresql/ -x \ && rm /tmp/metastore_db.zstd ``` I guess Option 1 is preferred but wanted to also mention the idea in Option 2. ########## standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/postgresql.conf: ########## @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# general + +listen_addresses = '*' +dynamic_shared_memory_type = posix + +# performance + +work_mem = 256MB +shared_buffers = 128MB + +# reduce WAL and archiving + +checkpoint_timeout=1d +max_wal_size = 64MB +min_wal_size = 32MB +wal_level = minimal +max_wal_senders = 0 +wal_buffers = -1 Review Comment: It was somewhat strange to see these messages on startup since since there were no queries actively running on the database. I guess it is somewhat related to how the dump and restore was performed. Apart from that I am not sure if wal_size has an effect on this use-case. During the TPC-DS test runs there are hardly any write queries so I would expect that WAL does not move a lot. It's fine with me if you want to keep them but more configs increase maintenance overhead since if someone wants to change (or remove) them in the future they will have to understand how important they were in the first place. ########## standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/postgresql.conf: ########## @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# general + +listen_addresses = '*' +dynamic_shared_memory_type = posix + +# performance + +work_mem = 256MB Review Comment: I guess we have to monitor the memory of the process while running the TPC-DS suite and pick the value accordingly. We just have to be careful not to change too much the memory usage of the pod cause things may start failing if we exceed the limits (~11GB for the three containers). ########## standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/Dockerfile: ########## @@ -14,9 +14,16 @@ # See the License for the specific language governing permissions and # limitations under the License. # -FROM postgres:12.3 +FROM postgres:18-alpine + +ADD https://github.com/thomasrebele/hive-postgres-metastore/releases/download/tpcds-30tb-histogram-1.0/metastore_tpcds30tb_with_histograms.raw_db.zstd /tmp/metastore_db.zstd Review Comment: Let's postpone the migration to https://github.com/apache/hive-test-datasets till https://issues.apache.org/jira/browse/LEGAL-738 is resolved. We can continue using your account for the moment. ########## standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/postgresql.conf: ########## @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# general + +listen_addresses = '*' +dynamic_shared_memory_type = posix + +# performance + +work_mem = 256MB +shared_buffers = 128MB Review Comment: Maintenance is easier if we only deal with settings and confs that make impact on the use-case. When we explicitly set configurations it gives the impression that the settings are important. For instance, I spend quite a bit of time trying to understand which of these configurations are important for the test speedup highlighted at the description of the PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
